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

Issue 29736629: Issue 6532 - Removes required subscription title input field (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 months, 3 weeks ago by Jon Sonesen
Modified:
4 months ago
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: 8

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: 12
Jon Sonesen
5 months, 3 weeks 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 ...
5 months, 3 weeks 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 ...
5 months, 2 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 ...
5 months, 2 weeks 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 ...
5 months, 2 weeks 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 ...
5 months, 1 week 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 ...
5 months, 1 week ago (2018-04-09 11:26:59 UTC) #7
Jon Sonesen
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#oldcode1232 ...
5 months, 1 week ago (2018-04-09 20:40:28 UTC) #8
Thomas Greiner
LGTM If you send me the patch I can push it for you to our ...
4 months, 3 weeks ago (2018-04-27 09:48:47 UTC) #9
wspee
We updated the spec (sorry about that, I think it wasn't clear that was already ...
4 months, 2 weeks ago (2018-05-02 08:14:09 UTC) #10
Thomas Greiner
Too late, I pushed the change a few minutes ago (see https://gitlab.com/eyeo/adblockplus/adblockplusui/commit/f3b8edbf8ab20e9a1dc40a12e1eefa2249f7e24c) so let's tackle ...
4 months, 2 weeks ago (2018-05-02 10:48:44 UTC) #11
Thomas Greiner
4 months, 2 weeks ago (2018-05-02 11:10:37 UTC) #12
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.

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