-
Notifications
You must be signed in to change notification settings - Fork 974
Check input value before set it #9031
Conversation
commonStyles.textbox, | ||
commonStyles.textbox__outlineable, | ||
commonStyles.isCommonForm, | ||
commonFormStyles.input__box |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would make these into a const to avoid repetitive input forms below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in 4652c1f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix works nicely.
@@ -97,12 +97,16 @@ class AddEditBookmarkHanger extends ImmutableComponent { | |||
this.updateFolders(nextProps) | |||
} | |||
} | |||
componentDidUpdate () { | |||
console.log(this.displayBookmarkName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops- we'll want to take this out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pulled this out in a separate commit and rebased/squashed it in 😄
fix #9017 Auditors: @bsclifton, @bbondy Test Plan: 1. Open editBookmark/autofill address/autofill credit card dialog 2. Make sure IME can input appropriately on text input value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have IME enabled (and I haven't used it on macOS before), so I can't easily test this. However, the changes look reasonable and I trust Suguru as he opened the issue 😄
It would be interesting to think about (in the future) a way to have an integration test that uses IME... for autofill or typing in the URL bar. We couldn't include it as part of the standard tests since not everybody has their system/locale setup in a way that enables IME, but we could have a dedicated machine for setting the locale / running the tests
Test Plan:
Description
fix #9017
Auditors: @bsclifton, @bbondy
Submitter Checklist:
git rebase -i
to squash commits (if needed).Reviewer Checklist:
Tests