Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2670963003:
MD Settings: in spell-check page, disable add button when input is empty. (Closed)
|
Created:
3 years, 10 months ago by scottchen Modified:
3 years, 10 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: in spell-check page, disable add button when input is empty
BUG=684152
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2670963003
Cr-Commit-Position: refs/heads/master@{#448524}
Committed: https://chromium.googlesource.com/chromium/src/+/1a0c5b4685de5be346d50768882780b3a634fbd2
Patch Set 1 #
Total comments: 6
Patch Set 2 : fixes based on comments #Patch Set 3 : add edit-dictionary page test #
Total comments: 2
Patch Set 4 : gets rid of confusing selector #
Total comments: 5
Messages
Total messages: 30 (14 generated)
Description was changed from ========== MD Settings: in spell-check page, disable add button when input is empty BUG=684152 ========== to ========== MD Settings: in spell-check page, disable add button when input is empty BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
scottchen@chromium.org changed reviewers: + dpapad@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Normally I suggest adding a test, unless you think it is not worth it. Is it worth it? https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:14: newWordValue: String, Nit (optional): Perhaps rename s/newWordValue/newWord/ I think it is fine since one is an ID and one is a Polymer property. https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:40: * @return {bool} true if value is empty, false otherwise. This will blow up in compilation. s/bool/boolean. Also @private missing. https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:42: isNewWordEmpty_: function(value) { I suggest not capturing the implementation details of this function in the name, just the concept, which is validation. s/isNewWordEmpty_/validateWord_ (you also need to reverse the returned boolean, so that true means valid).
Will add a test in the next patch https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:14: newWordValue: String, On 2017/02/04 01:13:16, dpapad wrote: > Nit (optional): Perhaps rename s/newWordValue/newWord/ > I think it is fine since one is an ID and one is a Polymer property. I'm gonna keep this the same; I changed it to newWord and I got confused myself, haha. https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:40: * @return {bool} true if value is empty, false otherwise. On 2017/02/04 01:13:16, dpapad wrote: > This will blow up in compilation. s/bool/boolean. Also @private missing. Done. https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:42: isNewWordEmpty_: function(value) { On 2017/02/04 01:13:16, dpapad wrote: > I suggest not capturing the implementation details of this function in the name, > just the concept, which is validation. > > s/isNewWordEmpty_/validateWord_ > (you also need to reverse the returned boolean, so that true means valid). Done.
On 2017/02/06 at 18:44:01, scottchen wrote: > Will add a test in the next patch > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js (right): > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:14: newWordValue: String, > On 2017/02/04 01:13:16, dpapad wrote: > > Nit (optional): Perhaps rename s/newWordValue/newWord/ > > I think it is fine since one is an ID and one is a Polymer property. > > I'm gonna keep this the same; I changed it to newWord and I got confused myself, haha. > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:40: * @return {bool} true if value is empty, false otherwise. > On 2017/02/04 01:13:16, dpapad wrote: > > This will blow up in compilation. s/bool/boolean. Also @private missing. > > Done. > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:42: isNewWordEmpty_: function(value) { > On 2017/02/04 01:13:16, dpapad wrote: > > I suggest not capturing the implementation details of this function in the name, > > just the concept, which is validation. > > > > s/isNewWordEmpty_/validateWord_ > > (you also need to reverse the returned boolean, so that true means valid). > > Done. Did you forget to upload a new patch?
The CQ bit was checked by scottchen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/02/06 18:45:51, dpapad wrote: > On 2017/02/06 at 18:44:01, scottchen wrote: > > Will add a test in the next patch > > > > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > > File chrome/browser/resources/settings/languages_page/edit_dictionary_page.js > (right): > > > > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:14: > newWordValue: String, > > On 2017/02/04 01:13:16, dpapad wrote: > > > Nit (optional): Perhaps rename s/newWordValue/newWord/ > > > I think it is fine since one is an ID and one is a Polymer property. > > > > I'm gonna keep this the same; I changed it to newWord and I got confused > myself, haha. > > > > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:40: * > @return {bool} true if value is empty, false otherwise. > > On 2017/02/04 01:13:16, dpapad wrote: > > > This will blow up in compilation. s/bool/boolean. Also @private missing. > > > > Done. > > > > > https://codereview.chromium.org/2670963003/diff/1/chrome/browser/resources/se... > > chrome/browser/resources/settings/languages_page/edit_dictionary_page.js:42: > isNewWordEmpty_: function(value) { > > On 2017/02/04 01:13:16, dpapad wrote: > > > I suggest not capturing the implementation details of this function in the > name, > > > just the concept, which is validation. > > > > > > s/isNewWordEmpty_/validateWord_ > > > (you also need to reverse the returned boolean, so that true means valid). > > > > Done. > > Did you forget to upload a new patch? Was just working on it, sorry. Next time I'll publish after the patch is uploaded.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2670963003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2670963003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:458: spellCheckCollapse.querySelector('.list-button:last-of-type')); Are you sure that this selector works as expected? AFAIK first-of-type, last-of-type can only used with element tags, not CSS clases, for example div:last-of-type OK p: first-of-type OK .foo:last-of-type NOT OK
https://codereview.chromium.org/2670963003/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2670963003/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:458: spellCheckCollapse.querySelector('.list-button:last-of-type')); On 2017/02/06 22:40:59, dpapad wrote: > Are you sure that this selector works as expected? AFAIK first-of-type, > last-of-type can only used with element tags, not CSS clases, for example > > div:last-of-type OK > p: first-of-type OK > .foo:last-of-type NOT OK The (non-)existence of the subpage is checked before and after the target element is mock-tapped, so I think it's selecting the collapse button as intended. Also, :last-of-type seems to work with class. See: https://jsfiddle.net/dLwyqwwh/
> The (non-)existence of the subpage is checked before and after the target element is mock-tapped, so I think it's selecting the collapse button as intended. > Also, :last-of-type seems to work with class. See: https://jsfiddle.net/dLwyqwwh/ It seems to work accidentally, see a more refined example at https://jsfiddle.net/dLwyqwwh/2/.
LGTM https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:415: inputMethodsCollapse.querySelector('.list-button:last-of-type'); This reference to last-of-type as well as the one at line 119 are incorrectly assuming that it works with CSS classes, which does not seem to be accurate. Can you add a TODO, or just fix them in the same CL? https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:454: test('spellcheck edit dictionary page add word button', function() { Nit (optional): s/add word button/add word validation/
The CQ bit was checked by scottchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486430685686650,
"parent_rev": "91e95aaad89b56c05e40c1eb7eb6d189e6b34180", "commit_rev":
"1a0c5b4685de5be346d50768882780b3a634fbd2"}
Message was sent while issue was closed.
Description was changed from ========== MD Settings: in spell-check page, disable add button when input is empty BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: in spell-check page, disable add button when input is empty BUG=684152 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2670963003 Cr-Commit-Position: refs/heads/master@{#448524} Committed: https://chromium.googlesource.com/chromium/src/+/1a0c5b4685de5be346d507688827... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1a0c5b4685de5be346d507688827...
Message was sent while issue was closed.
dbeam@chromium.org changed reviewers: + dbeam@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:415: inputMethodsCollapse.querySelector('.list-button:last-of-type'); On 2017/02/07 00:47:16, dpapad wrote: > This reference to last-of-type as well as the one at line 119 are incorrectly > assuming that it works with CSS classes, which does not seem to be accurate. Can > you add a TODO, or just fix them in the same CL? scottchen@: it is customary to respond to reviewer CLs before landing, especially those that explicitly ask for this. if you and dpapad@ talked and decided on something out-of-band, that's fine, but mention it in this CL so it doesn't look like you've flat-out ignored his requests as your reviewer. i could see how a reviewer would not lg your cl without all nits addressed if stuff like this ^ happens often.
Message was sent while issue was closed.
On 2017/02/07 03:08:43, Dan Beam wrote: > https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... > File chrome/test/data/webui/settings/languages_page_browsertest.js (right): > > https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... > chrome/test/data/webui/settings/languages_page_browsertest.js:415: > inputMethodsCollapse.querySelector('.list-button:last-of-type'); > On 2017/02/07 00:47:16, dpapad wrote: > > This reference to last-of-type as well as the one at line 119 are incorrectly > > assuming that it works with CSS classes, which does not seem to be accurate. > Can > > you add a TODO, or just fix them in the same CL? > > scottchen@: it is customary to respond to reviewer CLs before landing, > especially those that explicitly ask for this. > > if you and dpapad@ talked and decided on something out-of-band, that's fine, but > mention it in this CL so it doesn't look like you've flat-out ignored his > requests as your reviewer. > > i could see how a reviewer would not lg your cl without all nits addressed if > stuff like this ^ happens often. Hey Dan, thanks for chiming in. I didn't actually see that comment until now, since the codereview tool view that I've been relying to see comments doesn't show it at all apparently (see: http://imgur.com/a/jnM3x). I'll change my workflow to rely on the email messages instead from now on. Thanks for the heads up!
Message was sent while issue was closed.
https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... File chrome/test/data/webui/settings/languages_page_browsertest.js (right): https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:415: inputMethodsCollapse.querySelector('.list-button:last-of-type'); On 2017/02/07 03:08:43, Dan Beam wrote: > On 2017/02/07 00:47:16, dpapad wrote: > > This reference to last-of-type as well as the one at line 119 are incorrectly > > assuming that it works with CSS classes, which does not seem to be accurate. > Can > > you add a TODO, or just fix them in the same CL? > > scottchen@: it is customary to respond to reviewer CLs before landing, > especially those that explicitly ask for this. > > if you and dpapad@ talked and decided on something out-of-band, that's fine, but > mention it in this CL so it doesn't look like you've flat-out ignored his > requests as your reviewer. > > i could see how a reviewer would not lg your cl without all nits addressed if > stuff like this ^ happens often. Sorry, missed this comment before landing. Will be addressed in new CL: https://codereview.chromium.org/2682893002 https://codereview.chromium.org/2670963003/diff/60001/chrome/test/data/webui/... chrome/test/data/webui/settings/languages_page_browsertest.js:454: test('spellcheck edit dictionary page add word button', function() { On 2017/02/07 00:47:16, dpapad wrote: > Nit (optional): s/add word button/add word validation/ Addressed in new CL: https://codereview.chromium.org/2682893002
Message was sent while issue was closed.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
