Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement select in Regions for big countries. #2

Merged
merged 1 commit into from Feb 26, 2019

Conversation

aljones15
Copy link
Contributor

  • Countries json added.
  • Eslint works.
  • Add a better countries json array with regions.
  • Fix a small error in the vue file.
  • Add a Region Selector for large countries.
  • Implement select in Regions for big countries.

BrAddressForm.vue Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
Copy link

@mattcollier mattcollier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit about jsdoc formatting.

BrAddressForm.vue Show resolved Hide resolved
states.js Outdated Show resolved Hide resolved
@mattcollier mattcollier self-requested a review February 22, 2019 23:35
@davidlehn
Copy link
Member

Where did the country data come from? Can we include it in this module with the current bedrock license?
Need to document how to check if it needs updating and how to do so, which would be needed from time to time.

@davidlehn
Copy link
Member

These may just be issues with the example, and are things that we can add issues for to address later:

  • zip/postal doesn't validate based on data in that countries file
  • the selector pop open animation time seems slow to me, would like it snappier
  • state selector ideally would allow 2 letter codes to be typed (auto expanded to full name perhaps)
  • keyboard only usage issues:
    • when you open selector with space, filter should be focused so you can start typing (no way to type anything like that I think, just nav with arrows?)
    • when tabbed over to country then typing should open selector and start the filter vs doing nothing
    • tab order is weird when you have to type out the state, then if you go select us it has the state selector. if you typed "VA" then when and picked US, it would clear the VA (needs full name to work)
  • future work:
    • add flags to region and countries ;-)
    • ensure country or similar data is dynamically loaded only when this component is actually displayed

@aljones15
Copy link
Contributor Author

aljones15 commented Feb 23, 2019

These may just be issues with the example, and are things that we can add issues for to address later:

  • zip/postal doesn't validate based on data in that countries file

feature is in the doc, but @gannan08 wanted the first one to be simple on choose USA or Canada you get a Region Select.

  • the selector pop open animation time seems slow to me, would like it snappier

I think that can reduced if it is a css animation a scoped class on the select should speed it up.
not seeing an option in the api: https://quasar-framework.org/components/select.html

  • add a scoped css class that decreases the css animation time.
  • state selector ideally would allow 2 letter codes to be typed (auto expanded to full name perhaps)

please keep in mind this is an internationalization feature, more USA specific features are not intended in the release. (also not all countries have 2 letter region codes). we could do a USA only selector in the future with this feature.

  • keyboard only usage issues:

    • when you open selector with space, filter should be focused so you can start typing (no way to type anything like that I think, just nav with arrows?)
  • auto focus on space.
  • when tabbed over to country then typing should open selector and start the filter vs doing nothing
  • on focus on selector keyboard should open and auto focus.
  • tab order is weird when you have to type out the state, then if you go select us it has the state selector. if you typed "VA" then when and picked US, it would clear the VA (needs full name to work)

  • future work:

    • add flags to region and countries ;-)
  • add this to the spec
  • ensure country or similar data is dynamically loaded only when this component is actually displayed

all the data is there on load actually and slow down is probably just css/javascript latency.

the data comes from https://www.geonames.org/countries/
we start with the countries data dump
then we make an api call to childrenJSON with the geonames id to get the ADMIN1 regions.
it's in the spec actually. I guess I could take my hack of this guy's project and abstract out the bits we need and turn it into an npm script we would need to make sure it doesn't end up on npm.

@davidlehn
Copy link
Member

It's fine to delay fixes and features until later. Please do. I just wanted to write things down quick. The dynamic load issue is not about display speed, it's about the extra network bytes for something rarely (?) used.

@aljones15 aljones15 force-pushed the enhancement/country-city-selector branch from 728d61b to 7a008d5 Compare February 25, 2019 01:05
@aljones15
Copy link
Contributor Author

aljones15 commented Feb 25, 2019

@davidlehn on enter or space Filter will be auto-focused. are you sure you want on any key? it might create confusion and potential conflicts with other handicap accessible features in a site. space and enter are fairly well know as alternatives to click, but shift or b g etc are not. I also renamed the commit you did not like, but did you mean that jsdoc-to-markdown just has no place in this project?

@aljones15
Copy link
Contributor Author

aljones15 commented Feb 25, 2019

@davidlehn p.s. that flag thing is not that hard to implement:

https://github.com/lipis/flag-icon-css

iso code is the name of the flag as an SVG.

added it to the spec.

@aljones15
Copy link
Contributor Author

aljones15 commented Feb 25, 2019

@gannan08 if you have time take a look.

@davidlehn @dlongley if you have a moment this is now 3 days old.

.eslintrc.js Show resolved Hide resolved
BrAddressForm.vue Outdated Show resolved Hide resolved
BrAddressForm.vue Outdated Show resolved Hide resolved
states.js Outdated Show resolved Hide resolved
countries.js Show resolved Hide resolved
@gannan08
Copy link
Contributor

@aljones15 When we select a specific country then we can contextualize the labels. For instance, the default region label which is "State/Province/Region" should be changed to "State" when used within a US context or "Province" when used in a CA context.

@aljones15
Copy link
Contributor Author

@aljones15 When we select a specific country then we can contextualize the labels. For instance, the default region label which is "State/Province/Region" should be changed to "State" when used within a US context or "Province" when used in a CA context.

trying to find this info 252 countries so not sure on how to categorize.
https://www.thoughtco.com/divisions-of-countries-1435411

there are a few of them.

@aljones15
Copy link
Contributor Author

@gannan08 adding country specific region names would require a lot more space in the json file and also possibly create confusion: https://en.wikipedia.org/wiki/List_of_administrative_divisions_by_country

Some names:

velaya'at
rayondis
estadoss
shěng
županije

I think the current Region, State or Province gets it across just fine.

BrAddressForm.vue Outdated Show resolved Hide resolved
BrAddressForm.vue Outdated Show resolved Hide resolved
@gannan08
Copy link
Contributor

I think the current Region, State or Province gets it across just fine.

Let's just go with Region, State, or Province then.

@aljones15
Copy link
Contributor Author

aljones15 commented Feb 26, 2019

I think the current Region, State or Province gets it across just fine.

Let's just go with Region, State, or Province then.

cool let me know if there are any further issues then with the PR.
@gannan08 do you approve of the PR now?

@aljones15 aljones15 force-pushed the enhancement/country-city-selector branch from b2e05ad to 9b384eb Compare February 26, 2019 19:18
@gannan08
Copy link
Contributor

cool let me know if there are any further issues then with the PR.

Yes, few issues. The countries list is not in alphabetical order. Found Switzerland listed next to countries starting with "C". Are we not implementing the "State" and "Province" label for "USA" and "Canada" respectively?

@gannan08 do you approve of the PR now?

Not yet.

@gannan08
Copy link
Contributor

@aljones15 Do not worry about any outstanding issues on this PR. I will go ahead and resolve those issues then merge this down.

@aljones15
Copy link
Contributor Author

@aljones15 Do not worry about any outstanding issues on this PR. I will go ahead and resolve those issues then merge this down.

wait a few minutes please will ping when ready.

@aljones15 aljones15 force-pushed the enhancement/country-city-selector branch from 9b384eb to 66ee9b1 Compare February 26, 2019 20:36
@davidlehn
Copy link
Member

It's more common to have a "bin" dir that has installable binaries for the package (as listed in packge.json). No rush on this, but maybe that should move to a different name. "utils" or "updater" or something.

@aljones15
Copy link
Contributor Author

@aljones15 Do not worry about any outstanding issues on this PR. I will go ahead and resolve those issues then merge this down.

wait a few minutes please will ping when ready.

@gannan08 ok should be good for a rebase now. issue it turns out if that one country returned from geonames had no name or iso which meant that on search you would get errors etc. should be alphabetized now and working.

@gannan08
Copy link
Contributor

It's more common to have a "bin" dir that has installable binaries for the package (as listed in packge.json). No rush on this, but maybe that should move to a different name. "utils" or "updater" or something.

The bin/ directory should have only included the script needed to generate the countries file. This will be fixed.

@davidlehn
Copy link
Member

geonames is CC-BY. Need to note this in the README and probably have appropriate text in the LICENSE file.

@aljones15
Copy link
Contributor Author

It's more common to have a "bin" dir that has installable binaries for the package (as listed in packge.json). No rush on this, but maybe that should move to a different name. "utils" or "updater" or something.

The bin/ directory should have only included the script needed to generate the countries file. This will be fixed.

I actually was thinking that the actual .vue file that npm receives should not have that code in it or it's depencies. hence moved the entire project inside it.

@gannan08
Copy link
Contributor

I actually was thinking that the actual .vue file that npm receives should not have that code in it or it's depencies. hence moved the entire project inside it.

If by "actual .vue" file you mean the address form component, then it does not receive the contents of the bin/ directory because it should not be included in that component. The dependencies for the script will not be installed either. When this package is installed by other packages the dependencies for that script should be listed in devDependencies.

@gannan08 gannan08 force-pushed the enhancement/country-city-selector branch 4 times, most recently from 02606f1 to 0408e13 Compare February 26, 2019 22:13
@gannan08
Copy link
Contributor

@davidlehn @dlongley Can I get a review?

@gannan08 gannan08 force-pushed the enhancement/country-city-selector branch from 0408e13 to 8ec7d75 Compare February 26, 2019 22:23
@gannan08 gannan08 merged commit 021d2ce into dev Feb 26, 2019
@gannan08 gannan08 deleted the enhancement/country-city-selector branch February 26, 2019 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants