Issue 6532 - Removes required subscription title input field

Issue 6532 - Removes required subscription title input field

[Jon Sonesen, 2018-03-30 03:37:19]

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.

[Thomas Greiner, 2018-04-03 14:11:04]

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."

[Jon Sonesen, 2018-04-06 01:10:10]

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.

[Thomas Greiner, 2018-04-06 13:57:27]

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.

[Jon Sonesen, 2018-04-09 04:12:02]

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.

[Thomas Greiner, 2018-04-09 11:26:59]

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`.

[Jon Sonesen, 2018-04-09 20:40:28]

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.