Issue 29736629: Issue 6532 - Removes required subscription title input field (Closed)
|
Created:
5 months, 3 weeks ago by Jon Sonesen Modified:
4 months ago Base URL:
https://hg.adblockplus.org/adblockplusui Visibility:
Public. |
DescriptionIssue 6532 - Removes required subscription title input field
Patch Set 1 #
Total comments: 9
Patch Set 2 : Address PS1 comments #
Total comments: 8
Patch Set 3 : Address PS2 comments #
MessagesTotal messages: 12
https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html File desktop-options.html (left): https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:407: <p class="floating-input"> One thing I was thinking, is that maybe this doesn't need to be a popup floating input now that it only requires a single input field.
Sign in to reply to this message.
Just a couple of comments to make sure we don't break anything. https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html File desktop-options.html (left): https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:402: <input placeholder=" " id="import-list-title" type="text" class="default-focus" required /> We're still trying to access its value in js/desktop-options.js so removing the HTML element is only half the work. https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:402: <input placeholder=" " id="import-list-title" type="text" class="default-focus" required /> You're removing an element with the class "default-focus" so there's no element anymore which will be focused when opening the dialog. Therefore let's move this class to `#import-list-url`. https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:407: <p class="floating-input"> On 2018/03/30 03:37:19, Jon Sonesen wrote: > One thing I was thinking, is that maybe this doesn't need to be a popup floating > input now that it only requires a single input field. "Floating input" doesn't refer to CSS's "float" property but to the input title floating above the input field when focusing it. https://codereview.adblockplus.org/29736629/diff/29736630/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29736629/diff/29736630/locale/en_US/deskto... locale/en_US/desktop-options.json:1: { It's sufficient to update strings in locale/en_US/*.json files. No need to touch any other files in locale/ because those will be added/updated/removed automatically when we sync translations. https://codereview.adblockplus.org/29736629/diff/29736630/locale/en_US/deskto... locale/en_US/desktop-options.json:448: } Coding style: "Newline at end of file, otherwise no trailing whitespace."
Sign in to reply to this message.
https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html File desktop-options.html (left): https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:402: <input placeholder=" " id="import-list-title" type="text" class="default-focus" required /> On 2018/04/03 14:11:04, Thomas Greiner wrote: > We're still trying to access its value in js/desktop-options.js so removing the > HTML element is only half the work. Acknowledged. https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.htm... desktop-options.html:407: <p class="floating-input"> On 2018/04/03 14:11:04, Thomas Greiner wrote: > On 2018/03/30 03:37:19, Jon Sonesen wrote: > > One thing I was thinking, is that maybe this doesn't need to be a popup > floating > > input now that it only requires a single input field. > > "Floating input" doesn't refer to CSS's "float" property but to the input title > floating above the input field when focusing it. Acknowledged. https://codereview.adblockplus.org/29736629/diff/29736630/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29736629/diff/29736630/locale/en_US/deskto... locale/en_US/desktop-options.json:1: { On 2018/04/03 14:11:04, Thomas Greiner wrote: > It's sufficient to update strings in locale/en_US/*.json files. No need to touch > any other files in locale/ because those will be added/updated/removed > automatically when we sync translations. Acknowledged.
Sign in to reply to this message.
Added two more comments about non-obvious side effects. https://codereview.adblockplus.org/29736629/diff/29741693/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29736629/diff/29741693/desktop-options.htm... desktop-options.html:403: <label for="import-list-url" class="default-focus i18n_options_dialog_import_subscription_location"></label> Detail: The "default-focus" class used to be assigned to the `<input>` element, not to the `<label>` element. While it may work as is, note that our translation resolution logic is quite hacky which means that "i18n_*" classes only work when they are placed at the very beginning of the "class" attribute. Therefore please move the class to the element above. https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.js File js/desktop-options.js (left): https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.... js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) Please don't change the signature of this function. It's being used by other parts of the code where we do pass a title. Therefore those parts would break if we remove this argument. https://codereview.adblockplus.org/29736629/diff/29741693/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29736629/diff/29741693/locale/en_US/deskto... locale/en_US/desktop-options.json:426: "options_dialog_language_title": { Detail: This whitespace appears to have been added by accident so let's undo this change.
Sign in to reply to this message.
Thanks for taking a look, just a question about the addEnableSubscription function. The other changes are done, just waiting on that :) https://codereview.adblockplus.org/29736629/diff/29741693/desktop-options.html File desktop-options.html (right): https://codereview.adblockplus.org/29736629/diff/29741693/desktop-options.htm... desktop-options.html:403: <label for="import-list-url" class="default-focus i18n_options_dialog_import_subscription_location"></label> On 2018/04/06 13:57:26, Thomas Greiner wrote: > Detail: The "default-focus" class used to be assigned to the `<input>` element, > not to the `<label>` element. > > While it may work as is, note that our translation resolution logic is quite > hacky which means that "i18n_*" classes only work when they are placed at the > very beginning of the "class" attribute. > > Therefore please move the class to the element above. Acknowledged. https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.js File js/desktop-options.js (left): https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.... js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) On 2018/04/06 13:57:27, Thomas Greiner wrote: > Please don't change the signature of this function. It's being used by other > parts of the code where we do pass a title. Therefore those parts would break if > we remove this argument. Acknowledged. I did dig around the code in this file I could not find anywhere else that uses a title, additionally I made sure to test the addition of new subscriptions and didn't see console errors. Also, the default titles appeared without error, perhaps I am missing something? https://codereview.adblockplus.org/29736629/diff/29741693/locale/en_US/deskto... File locale/en_US/desktop-options.json (right): https://codereview.adblockplus.org/29736629/diff/29741693/locale/en_US/deskto... locale/en_US/desktop-options.json:426: "options_dialog_language_title": { On 2018/04/06 13:57:27, Thomas Greiner wrote: > Detail: This whitespace appears to have been added by accident so let's undo > this change. Acknowledged.
Sign in to reply to this message.
https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.js File js/desktop-options.js (left): https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.... js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) On 2018/04/09 04:12:01, Jon Sonesen wrote: > Acknowledged. I did dig around the code in this file I could not find anywhere > else that uses a title, additionally I made sure to test the addition of new > subscriptions and didn't see console errors. Also, the default titles appeared > without error, perhaps I am missing something? It's called in line 584 as `addEnableSubscription(url, title)` where it's used to add a filter list via an abp-subscribe link because those links usually also contain the filter list title. So the only argument that seems to be redundant here is `homepage`.
Sign in to reply to this message.
On 2018/04/09 11:26:59, Thomas Greiner wrote: > https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.js > File js/desktop-options.js (left): > > https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.... > js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) > On 2018/04/09 04:12:01, Jon Sonesen wrote: > > Acknowledged. I did dig around the code in this file I could not find anywhere > > else that uses a title, additionally I made sure to test the addition of new > > subscriptions and didn't see console errors. Also, the default titles appeared > > without error, perhaps I am missing something? > > It's called in line 584 as `addEnableSubscription(url, title)` where it's used > to add a filter list via an abp-subscribe link because those links usually also > contain the filter list title. > > So the only argument that seems to be redundant here is `homepage`. Ah, indeed. When I was searching the file in vim i was using "EaddEnableSubscription" as my search term my bad. Thanks for explaining, not sure that removing the homepage arg is applicable to this commit rather it would be a noissue, but that's up to you assuming you even want to change that.
Sign in to reply to this message.
LGTM If you send me the patch I can push it for you to our Mercurial repo. https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.js File js/desktop-options.js (left): https://codereview.adblockplus.org/29736629/diff/29741693/js/desktop-options.... js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) > Thanks for explaining, not sure that removing the homepage arg is applicable to this commit rather it would be a noissue, but that's up to you assuming you even want to change that. No need to remove the "homepage" argument as part of this review.
Sign in to reply to this message.
We updated the spec (sorry about that, I think it wasn't clear that was already under review, hope it doesn't cause to many troubles) and may have added things that haven't been implemented in this patch (the auto focus feature maybe?). Could you please take a look at the spec and see if there is anything missing from the implementation <https://gitlab.com/eyeo/specs/spec/merge_requests/146>.
Sign in to reply to this message.
Too late, I pushed the change a few minutes ago (see https://gitlab.com/eyeo/adblockplus/adblockplusui/commit/f3b8edbf8ab20e9a1dc4...) so let's tackle that in a separate ticket.
Sign in to reply to this message.
I've looked into what has changed and seems like we're good: - The auto-focus change was spotted during the review and is included in the commit. - I've created https://gitlab.com/eyeo/adblockplus/adblockplusui/issues/73 to address the change for the subcribe link confirmation dialog title element.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
