Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(39)

Issue 29736629: Issue 6532 - Removes required subscription title input field

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 5 days ago by Jon Sonesen
Modified:
1 day, 1 hour ago
CC:
wspee
Base URL:
https://hg.adblockplus.org/adblockplusui
Visibility:
Public.

Description

Issue 6532 - Removes required subscription title input field

Patch Set 1 #

Total comments: 9

Patch Set 2 : Address PS1 comments #

Total comments: 7

Patch Set 3 : Address PS2 comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+3 lines, -18 lines) Patch
M desktop-options.html View 1 2 1 chunk +1 line, -7 lines 0 comments Download
M js/desktop-options.js View 1 2 1 chunk +1 line, -2 lines 0 comments Download
M locale/en_US/desktop-options.json View 1 2 1 chunk +1 line, -9 lines 0 comments Download

Messages

Total messages: 8
Jon Sonesen
3 weeks, 5 days ago (2018-03-30 03:31:11 UTC) #1
Jon Sonesen
https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html File desktop-options.html (left): https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html#oldcode407 desktop-options.html:407: <p class="floating-input"> One thing I was thinking, is that ...
3 weeks, 5 days ago (2018-03-30 03:37:19 UTC) #2
Thomas Greiner
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 ...
3 weeks ago (2018-04-03 14:11:04 UTC) #3
Jon Sonesen
https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html File desktop-options.html (left): https://codereview.adblockplus.org/29736629/diff/29736630/desktop-options.html#oldcode402 desktop-options.html:402: <input placeholder=" " id="import-list-title" type="text" class="default-focus" required /> On ...
2 weeks, 5 days ago (2018-04-06 01:10:10 UTC) #4
Thomas Greiner
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.html#newcode403 desktop-options.html:403: <label ...
2 weeks, 4 days ago (2018-04-06 13:57:27 UTC) #5
Jon Sonesen
Thanks for taking a look, just a question about the addEnableSubscription function. The other changes ...
2 weeks, 2 days ago (2018-04-09 04:12:02 UTC) #6
Thomas Greiner
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#oldcode1232 js/desktop-options.js:1232: function addEnableSubscription(url, title, homepage) On 2018/04/09 04:12:01, Jon Sonesen ...
2 weeks, 2 days ago (2018-04-09 11:26:59 UTC) #7
Jon Sonesen
2 weeks, 1 day ago (2018-04-09 20:40:28 UTC) #8
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5