Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
It's broken for non-AJAX callbacks as well, so that will be testable.
Comment | File | Size | Author |
---|---|---|---|
#12 | 1904932-12.patch | 2.67 KB | YesCT |
#12 | interdiff-11-12.txt | 960 bytes | YesCT |
#11 | 1904932-11.patch | 2.68 KB | damiankloip |
#11 | interdiff-1904932-11.txt | 3.23 KB | damiankloip |
#8 | 1904932-8.patch | 3.77 KB | damiankloip |
Comments
Comment #1
dawehnerComment #2
YesCT CreditAttribution: YesCT commentedmanually tested. And the js error is gone, but it jumps me out of the views overlay and wizard and to the front page.
tried to improve grammar of the comment. See if it still says the correct meaning of what you wanted it to say.
Comment #3
YesCT CreditAttribution: YesCT commentedComment #4
YesCT CreditAttribution: YesCT commentedComment #5
dawehnerSo we probably want to set this link to use an empty fragment, so it doesn't do anything for itself, without the javascript?
Comment #6
YesCT CreditAttribution: YesCT commented@dawehner How do we do that?
Also, this is "needs tests"
what kind of test should we add? webtest?
Comment #7
dawehnerWell, the removing just works on the javascript so I don't see how we should be able to test this?
The error mentioned by tim happens if you click on the link but the javascript fails.
Comment #8
damiankloip CreditAttribution: damiankloip commentedGave this a quick tidy up, now it matches alot of the other js code (I think :)). Also tested, and confirm the fix is good.
Removing needs tests too; Javascript is rubbish, and hard to test ;)
Comment #9
dawehnerThese aren't method documentations so should we use "//" instead?
Comment #10
olli CreditAttribution: olli commented.handlerRemoveLink -> .viewsUiHandlerRemoveLink
These could use the context param too.
Why not .once() instead of .addClass()?
Indentation.
Comment #11
damiankloip CreditAttribution: damiankloip commentedRerolled and added in feedback from #9 and #10, thanks. Its been a while!
Comment #12
YesCT CreditAttribution: YesCT commentedIn this js file, UIwhatever is more using the pattern: UiWhatever.
missed one. :)
Comment #13
YesCT CreditAttribution: YesCT commentedverified that the bug still exists without the patch.
manually tested with the patch:
Works great, stays in views ui, and I can save. Everything fine.
Next steps:
a code review (I should not rtbc as I did the most recent patch).
======
Actually, with and without this patch, rearranging the sort criteria or the no results behavior... results in all of those being deleted instead of rearranged. Rearranging the filter criteria works fine though. I dont see a issue for rearranging sort criteria bug (yet).
Should not stop this issue as it is a pre-existing bug.
Comment #14
YesCT CreditAttribution: YesCT commentedI talked to @damiankloip in irc.
rtbc!
Comment #15
tim.plunkettRTBC +1
Comment #16
alexpottCommitted f140bce and pushed to 8.x. Thanks!
I guess we need a new issue for the bug mentioned by @YesCT in #13
Comment #17
damiankloip CreditAttribution: damiankloip commentedI think #1929070: Refactor views_ui_rearrange_filter_form to remove theme function for table rendering is related to that, and will possibly/probably/maybe fix what is mentioned by YesCT.