Converts AutofillAddressPanel into redux component #9490

NejcZdovc · 2017-06-15T11:39:57Z

Submitter Checklist:

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).
Tagged reviewers and labelled the pull request as needed.

Resolves #9444

Auditors: @bsclifton @bridiver @darkdh

Test Plan:

try to add autofill address
try to delete it
try to edit it

Test Plan (l10n):

Install Google IME (CJK);
Enable it (Hiragana ひらがな mode)
Open about:autofill
Input brave
It should display the conversion candidate like this:

Reviewer Checklist:

Tests

Adequate test coverage exists to prevent regressions
Tests should be independent and work correctly when run individually or as a suite ref
New files have MPL2 license header

darkdh · 2017-06-15T17:20:00Z

app/renderer/components/autofill/autofillAddressPanel.js

  onSave () {
-    appActions.addAutofillAddress(this.props.currentDetail, this.props.originalDetail)


why removing this.props.currentDetail? This will cause save not working

because this is this WIP and I need to check some things before finishing this PR

what is the current status of this?

this is done, but we need to find a way to fix IME problem

darkdh · 2017-06-15T17:20:47Z

app/renderer/components/autofill/autofillAddressPanel.js

+    const originalDetail = currentWindow.getIn(['autofillAddressDetail', 'originalDetail'], Immutable.Map())
+
+    const props = {}
+    // Used in renderer


you will also need to set

props.currentDetail = currentDetail props.originalDetail = originalDetail

we don't want to do that, let's discuss it more on the call

darkdh

This change will introduce the problem of #9017

darkdh · 2017-06-16T17:05:15Z

app/autofill.js

@@ -15,16 +15,16 @@ module.exports.init = () => {

 module.exports.addAutofillAddress = (detail, guid) => {


guid arg here can be removed now

darkdh · 2017-06-16T17:12:17Z

test/contents/autofillTest.js

        .waitUntil(function () {
          return this.getAppState().then((val) => {
+            console.log(val.value.autofill.addresse)


forgot to remove?

NejcZdovc · 2017-06-16T17:38:11Z

This change will introduce the problem of #9017

Not sure what you meant by that

darkdh · 2017-06-16T18:01:22Z

just like this
https://cloud.githubusercontent.com/assets/3362943/26372992/d32887a0-403a-11e7-82fd-7568ecc15575.gif
The Chinese IME I use will have composition state same as Japanese.
And this will cause I couldn't select the word I need
it should let user to select word like this
https://cloud.githubusercontent.com/assets/3362943/26373228/a9d63644-403b-11e7-9285-74753cc846cf.gif

NejcZdovc · 2017-06-16T18:04:53Z

@darkdh if I understand correctly this PR will cause a regression for thing that was fixed in #9017, because I removed this part 4102e95#diff-590c7f19ab835ddb769d5c3e5b227ab5R51 ?

darkdh · 2017-06-16T18:07:25Z

yeah, but in this PR. Adding it back doesn't solve the issue because lots of things also changed

NejcZdovc · 2017-06-16T18:50:20Z

Yeah we can't just add that part back, because we are connected to the store now and it will not work, so we need to find another approach of how to fix this.

NejcZdovc · 2017-06-16T18:51:50Z

Will try to find a way how to fix it, added back label WIP

NejcZdovc · 2017-06-19T08:48:59Z

@darkdh can you please provide STR how to trigger this problem, because I tried it on my machine and I can't reproduce it, autofill dialog is not auto-dismissing

NejcZdovc · 2017-06-19T08:54:58Z

@luixxiul I saw that you tested out #9017, can you please checkout out this PR and see if you can reproduce this problem or not. I couldn't, but I am not sure that I did correct STR. Thank you

darkdh · 2017-06-19T15:41:22Z

@NejcZdovc you need an input method which requires selecting character for a certain symbol combination like Chinese or Japanese.
So the problem is it won't allow us to select the character. If we are in selecting stage, there will be underline on that character(s)

darkdh · 2017-06-19T15:48:42Z

I use Zhuyin on Mac. and you can have測試 by input hk4g4 and you can see underline on these two characters and you should be able to change character by arrow left/right to select character you want to change and arrow down is to change the character

luixxiul · 2017-06-19T15:48:53Z

Yes the issue gets back.

Also I added another test plan (l10n) to the first post.

ref #9045

NejcZdovc · 2017-06-20T08:17:29Z

@luixxiul @darkdh thank you so much for STR. I finally managed to reproduce it. I also pushed the fix for it, so can you please try it again?

darkdh

++

darkdh · 2017-06-20T14:46:37Z

app/renderer/components/autofill/autofillAddressPanel.js

-                ref={(nameOnAddress) => { this.nameOnAddress = nameOnAddress }} />
+                spellCheck='false'
+                ref={(ref) => { this.nameOnAddress = ref }}
+                defaultValue={this.props.name}


neat pick. The order is different than others

@bsclifton

Resolves brave#9444 Auditors: @bsclifton @bridiver Test Plan:

luixxiul · 2017-06-20T16:38:24Z

One slight issue on deleting the candidate.

STR:

Enable the Japanese IME
Input brave
Make sure you get ｂらう゛ぇ
Hit backspace 3 times
Make sure you only have ｂ
Hit that one more time to delete ｂ

Actual result: just conversion candidate pulldown disappears and ｂ is still there

Expected result: ｂ should be removed

@NejcZdovc would you mind solving this issue as it is a regression? thanks!

NejcZdovc added PR/work-in-progress ⚒ refactoring/redux-component labels Jun 15, 2017

NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 15, 2017

NejcZdovc self-assigned this Jun 15, 2017

NejcZdovc mentioned this pull request Jun 15, 2017

Redux refactoring tracking issue #8528

Closed

51 tasks

darkdh reviewed Jun 15, 2017

View reviewed changes

NejcZdovc force-pushed the redux/autofillAddressPanel branch 4 times, most recently from 6e6312c to a863d3b Compare June 15, 2017 20:21

NejcZdovc requested review from bridiver, bsclifton and darkdh June 15, 2017 20:22

NejcZdovc removed the PR/work-in-progress ⚒ label Jun 15, 2017

NejcZdovc mentioned this pull request Jun 16, 2017

Redux component - AutofillAddressPanel #9444

Closed

luixxiul added the feature/autofill label Jun 16, 2017

darkdh suggested changes Jun 16, 2017

View reviewed changes

NejcZdovc force-pushed the redux/autofillAddressPanel branch from a863d3b to 94a2942 Compare June 16, 2017 17:38

NejcZdovc requested a review from darkdh June 16, 2017 17:38

NejcZdovc added the PR/work-in-progress ⚒ label Jun 16, 2017

NejcZdovc removed the PR/work-in-progress ⚒ label Jun 19, 2017

luixxiul added l10n PR/needs-QA-attention ☕ labels Jun 19, 2017

NejcZdovc mentioned this pull request Jun 19, 2017

Converts AutofillCreditCardPanel into redux component #9566

Merged

8 tasks

NejcZdovc added the PR/work-in-progress ⚒ label Jun 19, 2017

NejcZdovc force-pushed the redux/autofillAddressPanel branch 2 times, most recently from fa21adb to 8a495f8 Compare June 20, 2017 08:16

NejcZdovc removed the PR/work-in-progress ⚒ label Jun 20, 2017

darkdh approved these changes Jun 20, 2017

View reviewed changes

NejcZdovc force-pushed the redux/autofillAddressPanel branch from 8a495f8 to fc54b2d Compare June 20, 2017 15:20

Converts AutofillAddressPanel into redux component

57d207a

Resolves brave#9444 Auditors: @bsclifton @bridiver Test Plan:

NejcZdovc force-pushed the redux/autofillAddressPanel branch from fc54b2d to 57d207a Compare June 20, 2017 15:22

NejcZdovc merged commit 759e164 into brave:master Jun 20, 2017

luixxiul mentioned this pull request Jun 20, 2017

IME conversion candidate remains in text field, including URL bar #9604

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converts AutofillAddressPanel into redux component #9490

Converts AutofillAddressPanel into redux component #9490

NejcZdovc commented Jun 15, 2017 •

edited by luixxiul

Loading

darkdh Jun 15, 2017

NejcZdovc Jun 15, 2017

bridiver Jun 19, 2017

NejcZdovc Jun 19, 2017

darkdh Jun 15, 2017

NejcZdovc Jun 15, 2017

darkdh left a comment

darkdh Jun 16, 2017

NejcZdovc Jun 16, 2017

darkdh Jun 16, 2017

NejcZdovc Jun 16, 2017

NejcZdovc commented Jun 16, 2017

darkdh commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

darkdh commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

NejcZdovc commented Jun 19, 2017 •

edited

Loading

NejcZdovc commented Jun 19, 2017 •

edited

Loading

darkdh commented Jun 19, 2017

darkdh commented Jun 19, 2017

luixxiul commented Jun 19, 2017 •

edited

Loading

NejcZdovc commented Jun 20, 2017 •

edited

Loading

darkdh left a comment

darkdh Jun 20, 2017

NejcZdovc Jun 20, 2017

luixxiul commented Jun 20, 2017

		onSave () {
		appActions.addAutofillAddress(this.props.currentDetail, this.props.originalDetail)

		@@ -15,16 +15,16 @@ module.exports.init = () => {

		module.exports.addAutofillAddress = (detail, guid) => {

Converts AutofillAddressPanel into redux component #9490

Converts AutofillAddressPanel into redux component #9490

Conversation

NejcZdovc commented Jun 15, 2017 • edited by luixxiul Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jun 16, 2017

darkdh commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

darkdh commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

NejcZdovc commented Jun 16, 2017

NejcZdovc commented Jun 19, 2017 • edited Loading

NejcZdovc commented Jun 19, 2017 • edited Loading

darkdh commented Jun 19, 2017

darkdh commented Jun 19, 2017

luixxiul commented Jun 19, 2017 • edited Loading

NejcZdovc commented Jun 20, 2017 • edited Loading

darkdh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul commented Jun 20, 2017

NejcZdovc commented Jun 15, 2017 •

edited by luixxiul

Loading

NejcZdovc commented Jun 19, 2017 •

edited

Loading

NejcZdovc commented Jun 19, 2017 •

edited

Loading

luixxiul commented Jun 19, 2017 •

edited

Loading

NejcZdovc commented Jun 20, 2017 •

edited

Loading