Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10300

dfperry5 · 2017-08-04T18:58:03Z

Fixes #9053

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.

Test Plan:

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

dfperry5 · 2017-08-04T19:03:11Z

@darkdh @bsclifton Created the PR... Not sure how to add you guys as reviewers :( Sorry for being a noob!

luixxiul · 2017-08-06T12:27:49Z

Is this l10n ready?

bsclifton · 2017-08-07T22:32:12Z

@dfperry5 awesome, nice work!

@darkdh added us both as reviewers- I should be able to review this week 😄

dfperry5 · 2017-08-07T22:59:41Z

@luixxiul Sorry to be a noob - l10n ready - whats that mean? Sorry - still have vacation brain!

@bsclifton Woohoo! let me know what else I need to do!

bsclifton · 2017-08-08T07:07:04Z

@dfperry5 l10n is a short way of saying localization (ex: L, then 10 letters, then N). You'll often see people do the same thing with i18n (internationalization), g11n (globalization), a11y (accessibility)

The countries you listed are in js/constants/countries.js. These names are perfect for English speakers, but folks using Brave in German would want to see the names of the countries in German (ex: Deutschland not Germany). We have some notes you can check out here:
https://github.com/brave/browser-laptop/blob/master/docs/translations.md

The translations themselves are handled through a 3rd party service (as described in the document). The part you'll want to read and reference is under "Making sure our code has all strings localized"

@luixxiul how would you suggest tackling this? Maybe we could make a separate countries.properties file and index the names by country code?

luixxiul · 2017-08-08T07:19:28Z

@luixxiul how would you suggest tackling this? Maybe we could make a separate countries.properties file and index the names by country code?

Yes, I think that would be the best way.

dfperry5 · 2017-08-09T01:46:15Z

@luixxiul oh gotcha. Makes perfect sense. I'll take a look at it and give it a shot. I'll prolly have a few other questions.

Thanks guys!

dfperry5 · 2017-08-11T01:38:10Z

@bsclifton @luixxiul

I attempted to fix the l1on stuff. But I can't seem to see it working on my local. Do you guys mind taking a loot at the above commit -> 0f93cc4 and letting me know what you think I am missing?

I even tried to add it in here: https://github.com/brave/browser-laptop/blob/master/app/locale.js, but I didn't think it really belonged because i wasnt sure if the autofill modal was considered a context menu or not.

Any feedback/help is appreciated!

Thanks!

dfperry5 · 2017-08-17T01:13:09Z

@bsclifton @luixxiul

Any help guys? :) I'm sure yall are busy! Just a reminder :D

luixxiul · 2017-08-17T07:55:51Z

Sorry, I don't know how to :-(

darkdh

@dfperry5 ,
you have to move <link rel="localization" href="locales/{locale}/countries.properties"> to
both app/extensions/brave/index.html and app/extensions/brave/index-dev.html

Also, the locale translation is case sensitive so you have to change you two country code in app/extensions/brave/locales/en-US/countries.properties to be upper case

dfperry5 · 2017-08-17T17:16:34Z

@darkdh Do you mind taking a look at my latest commit / pulling it down and trying for yourself. Sorry I'm struggling with this, haha. But it still doesn't seem like its working on my local.

Thanks!

darkdh

sorry, forgot to mention that you will also have to modify
app/locale.js

Add those country code to rendererIdentifiers
Add countries.properties to appendLangProperties

dfperry5 · 2017-08-17T17:40:18Z

@darkdh Updated it and it's working on my local now! thanks a ton!

darkdh · 2017-08-17T17:43:53Z

great and now there is another problem
about:autofill should look like this

but this PR shows

could you change it? 😉

codecov-io · 2017-08-17T17:58:46Z

Codecov Report

Merging #10300 into master will decrease coverage by 0.06%.
The diff coverage is 14.28%.

@@            Coverage Diff             @@
##           master   #10300      +/-   ##
==========================================
- Coverage   54.19%   54.12%   -0.07%     
==========================================
  Files         244      247       +3     
  Lines       21109    21550     +441     
  Branches     3258     3336      +78     
==========================================
+ Hits        11440    11665     +225     
- Misses       9669     9885     +216

Flag	Coverage Δ
#unittest	`54.12% <14.28%> (-0.07%)`	⬇️

Impacted Files	Coverage Δ
app/locale.js	`35.23% <0%> (-0.34%)`	⬇️
app/browser/menu.js	`52.19% <0%> (-0.55%)`	⬇️
js/l10n.js	`40.74% <50%> (-5.1%)`	⬇️
app/browser/reducers/tabsReducer.js	`43.24% <0%> (-6.92%)`	⬇️
app/common/lib/siteSuggestions.js	`89.92% <0%> (-2.88%)`	⬇️
js/state/frameStateUtil.js	`54.27% <0%> (-1.64%)`	⬇️
js/stores/windowStore.js	`28.59% <0%> (-1.19%)`	⬇️
js/about/preferences.js	`42.32% <0%> (-1.06%)`	⬇️
...components/preferences/payment/bitcoinDashboard.js	`21.46% <0%> (-0.38%)`	⬇️
... and 31 more

dfperry5 · 2017-08-17T18:12:00Z

@darkdh done -- i think :)

dfperry5 · 2017-08-17T18:46:50Z

@darkdh Alrighty - look at that last commit and see if it looks OK :)

Thanks for all the help/patience!

darkdh · 2017-08-17T19:51:45Z

if you test locally, you will find out it didn't work because you have to add <link rel="localization" href="locales/{locale}/countries.properties"> to app/extensions/brave/about-autofill.html

dfperry5 · 2017-08-17T21:11:40Z

@darkdh I thought I did that right here: 0f93cc4.

I already added it in there :) Unless thats not what you mean?

Sorry for all the questions!

darkdh · 2017-08-17T21:41:27Z

ok. my bad. I didn't see that. you are doing fine.

bsclifton

OK @dfperry5 - I found and fixed the issues I saw (they're not related to your changes). We have an active tab bug (which @bbondy is fixing) which triggered the error I saw

I tried the PR and only noticed one thing 😄

If you go to add an address, and just type in your name and pick "Save", country is not saved (even though it shows Afghanistan as the selected value). Can the dropdown also have an empty value which it defaults to? This way, the user has to pick the country. This is useful also if the user wants to clear the country that was entered

dfperry5 · 2017-08-23T00:21:52Z

@bsclifton I'll take a look! Appreciate all the help!

bsclifton · 2017-09-01T21:16:39Z

@dfperry5 hey- I just wanted to check back on this. Did you have a chance to check it out? Let us know if you need a hand 😄

dfperry5 · 2017-09-04T20:53:21Z

@bsclifton Sorry! Time got away from me :) Started up grad school + work - looking at it right now.

dfperry5 · 2017-09-04T21:01:59Z

@bsclifton Pushed my changes :) Let me know what you think!

bsclifton

Looks (and works) great! 😄 👍

@darkdh maybe you can give this a shot again and if you approve, we'll be ready to merge 😄

dfperry5 · 2017-09-05T20:34:44Z

@bsclifton woohoo! :) Let me know what the next steps are to get this merged in!

NejcZdovc · 2017-09-06T10:12:33Z

changing this back to 0.22.x, because issue is set to 0.22.x as well. @bsclifton if you want to land this in 0.21.x please change milestone back

luixxiul · 2017-09-06T10:22:37Z

after merged the milestone will be reset to 0.21.x anyway.

luixxiul · 2017-09-06T10:34:47Z

app/renderer/components/autofill/autofillAddressPanel.js

+    width: '100%'
+  },
+
+  sectionWrapper__expirationDate: {


Is this necessary?

luixxiul · 2017-09-06T10:35:37Z

app/renderer/components/autofill/autofillAddressPanel.js

+  expirationDate__dropdowns: {
+    display: 'flex'
+  },
+  dropdown__right: {


luixxiul · 2017-09-06T10:36:20Z

app/renderer/components/autofill/autofillAddressPanel.js

+
+const styles = StyleSheet.create({
+  // Copied from textbox.js
+  input: {


luixxiul

See my comment above.

NejcZdovc · 2017-09-06T11:14:23Z

@luixxiul not necessary, because if PR is approved and we set milestone to 0.22.x, we will wait with merge until master is set to 0.22.x

luixxiul · 2017-09-06T11:17:24Z

confused- I added this PR to 0.22.x before @bsclifton set the same milestone to the issue.

luixxiul · 2017-09-06T11:19:27Z

oops...sorry I mistakenly closed this PR! sorry!

luixxiul · 2017-09-06T11:20:56Z

I'm going to cherry-pick the commits and create a new PR. Apologies to @dfperry5 :-(

dfperry5 · 2017-09-06T11:29:05Z

@luixxiul all good :) I'll look at those changes you requested this afternoon!

luixxiul · 2017-09-06T11:31:21Z

@dfperry5 I opened a new PR by picking the commits you created. Please see #10821, thanks!

darkdh requested review from bsclifton and darkdh August 4, 2017 21:30

luixxiul added feature/autofill needs-info Another team member needs information from the PR/issue opener. labels Aug 6, 2017

luixxiul added this to the 0.22.x milestone Aug 6, 2017

bsclifton assigned dfperry5 Aug 7, 2017

bsclifton added the l10n label Aug 8, 2017

bsclifton requested a review from luixxiul August 8, 2017 07:16

luixxiul removed the needs-info Another team member needs information from the PR/issue opener. label Aug 8, 2017

darkdh reviewed Aug 17, 2017

View reviewed changes

bsclifton suggested changes Aug 22, 2017

View reviewed changes

bsclifton approved these changes Sep 5, 2017

View reviewed changes

luixxiul requested a review from darkdh September 6, 2017 09:57

luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.22.x Sep 6, 2017

NejcZdovc modified the milestones: 0.22.x, 0.21.x (Nightly Channel) Sep 6, 2017

luixxiul reviewed Sep 6, 2017

View reviewed changes

luixxiul suggested changes Sep 6, 2017

View reviewed changes

luixxiul closed this Sep 6, 2017

luixxiul force-pushed the master branch from 454747b to 2014e8c Compare September 6, 2017 11:18

luixxiul mentioned this pull request Sep 6, 2017

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10821

Merged

8 tasks

luixxiul removed this from the 0.22.x milestone Sep 6, 2017

luixxiul removed the request for review from darkdh September 6, 2017 11:46

luixxiul mentioned this pull request Sep 6, 2017

Make Autofill country field into a dropdown list #9053

Closed

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10300

Issue #9053: Making country field in autofill a dropdown list instead of text input box. #10300

Conversation

dfperry5 commented Aug 4, 2017 • edited Loading

dfperry5 commented Aug 4, 2017

luixxiul commented Aug 6, 2017

bsclifton commented Aug 7, 2017

dfperry5 commented Aug 7, 2017

bsclifton commented Aug 8, 2017 • edited Loading

luixxiul commented Aug 8, 2017

dfperry5 commented Aug 9, 2017

dfperry5 commented Aug 11, 2017

dfperry5 commented Aug 17, 2017

luixxiul commented Aug 17, 2017

darkdh left a comment

Choose a reason for hiding this comment

dfperry5 commented Aug 17, 2017

darkdh left a comment

Choose a reason for hiding this comment

dfperry5 commented Aug 17, 2017

darkdh commented Aug 17, 2017

codecov-io commented Aug 17, 2017 • edited Loading

Codecov Report

dfperry5 commented Aug 17, 2017

dfperry5 commented Aug 17, 2017

darkdh commented Aug 17, 2017

dfperry5 commented Aug 17, 2017

darkdh commented Aug 17, 2017

bsclifton left a comment • edited Loading

Choose a reason for hiding this comment

dfperry5 commented Aug 23, 2017

bsclifton commented Sep 1, 2017

dfperry5 commented Sep 4, 2017

dfperry5 commented Sep 4, 2017

bsclifton left a comment

Choose a reason for hiding this comment

dfperry5 commented Sep 5, 2017

NejcZdovc commented Sep 6, 2017

luixxiul commented Sep 6, 2017

luixxiul Sep 6, 2017

Choose a reason for hiding this comment

luixxiul Sep 6, 2017

Choose a reason for hiding this comment

luixxiul Sep 6, 2017

Choose a reason for hiding this comment

luixxiul left a comment

Choose a reason for hiding this comment

NejcZdovc commented Sep 6, 2017

luixxiul commented Sep 6, 2017

luixxiul commented Sep 6, 2017

luixxiul commented Sep 6, 2017

dfperry5 commented Sep 6, 2017

luixxiul commented Sep 6, 2017

dfperry5 commented Aug 4, 2017 •

edited

Loading

bsclifton commented Aug 8, 2017 •

edited

Loading

codecov-io commented Aug 17, 2017 •

edited

Loading

bsclifton left a comment •

edited

Loading