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

feat: implement persisted country selection; deps for writing tests #94

Closed
wants to merge 21 commits into from

Conversation

piperchester
Copy link
Contributor

@piperchester piperchester commented Oct 24, 2020

This implements #28. I use window.sessionStorage to persist the selected country as well as React's state to trigger a client-side refresh when the user selects and clears the option.

selected-country

Open to any suggestions, big or small on any of this!

Also

  • Added first .test.js file, __snapshots__ and necessary Jest deps
  • Added selected-item class to scope the white color for the country
  • Added PR template
  • Fixed console warnings in RecentLinkItem

Todo

  • Tests
  • Clean up
  • Do not default to a country

@piperchester piperchester changed the title feat: add language selection feat: implement persisted language selection; deps for writing tests Oct 24, 2020
@piperchester piperchester marked this pull request as ready for review October 24, 2020 21:45
@piperchester
Copy link
Contributor Author

Hey Phil and Mike! Requested you as reviewers as I'm not sure who the best folks to reach out to are. Happy to elaborate on any of this!

src/components/LanguageDropdown.jsx Outdated Show resolved Hide resolved
src/components/LanguageDropdown.test.js Outdated Show resolved Hide resolved
src/components/RecentLinkItem.jsx Show resolved Hide resolved
@philsturgeon
Copy link
Collaborator

Thanks for taking a swing at this!

Sorry for any confusion, but this is only meant to be a country selector. Everyone is still stuck seeing english, it's just going to display content for Brazil or Australia or Bolivia, it's all about restricting to content relevant to you. A basic filter, which is persisted.

Localiztion is out of scope, as #14 will give people language. If somebody else wants to view the site in French when they live in Mongolia that should be fine.

Do you think you could tweak that to make it clear?

@piperchester
Copy link
Contributor Author

Do you think you could tweak that to make it clear?

For sure! So is the intent to fold the functionality from the "Filter site for your country" bit into a single dropdown?

Also good timing I think - guessing the site's undergoing some maintenance but getting a "website is pending domain owner verification" on https://protect.earth/.

@piperchester piperchester marked this pull request as draft October 25, 2020 18:28
@philsturgeon
Copy link
Collaborator

Right, the goal was to make the "Filter site for your country" work properly.

  1. persist the country of choice
  2. display the country of choice
  3. make sure it showed things from your country and things which had no country, because a website available anywhere is a good thing to show to somebody wherever they are

Does that make sense?

The hosted URL is https://actions.protect.earth whilst we switch the main page over to Squarespace, which is surprisingly slow at making SSL certificates. 😓

"editor.defaultFormatter": "esbenp.prettier-vscode",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to automatically invoke prettier on save. This streamlines my DX but can definitely pull this out if undesired!

Comment on lines +14 to +19
// Get the country code. e.g., "BG" for "Bulgaria"
const [country] = useState(sessionStorage.getItem('country_pref'));

// Default to Great Britain
const countryName = Countries.fromAlpha2Code(country || 'GB').name;
const countryEmoji = Countries.fromAlpha2Code(country || 'GB').emoji;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Always look into the session storage first. Defaults to 'GB' which I could see being controversial. @ reviewers, wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cn we make the default be no country? The idea of a filter is that it reduces the overall data to a subset of more relevant information, but that is an optional feature which is only used when folks need it.

Copy link
Contributor Author

@piperchester piperchester Nov 9, 2020

Choose a reason for hiding this comment

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

Sure thing! Will fix this 👍. I'll continually update the "TODO" section in the PR description and toggle between ready / draft.

Comment on lines +19 to +31
/**
* When a country is selected, we store a
* temporary "country_pref" value in session storage
* that persists through reloads. This enables users to have
* continuity when refreshing, or returning to the site.
*
* We also bind the update to React's state, so that when a user re-selects
* an option, we trigger a state change to refresh the site.
*/
const updateCountry = (country) => {
setCountry(country);
sessionStorage.setItem('country_pref', country);
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the core piece where we write to session storage, and update React's state.

@piperchester piperchester marked this pull request as ready for review November 8, 2020 00:32
@piperchester piperchester changed the title feat: implement persisted language selection; deps for writing tests feat: implement persisted country selection; deps for writing tests Nov 8, 2020
@piperchester
Copy link
Contributor Author

Took another stab at this @philsturgeon! Happy to make any desired changes.

@piperchester piperchester marked this pull request as draft November 9, 2020 15:45
@piperchester
Copy link
Contributor Author

Getting a chance to revisit this @philsturgeon - apologies for the delays!

Following the contrib guidelines, do you mind sharing env variables for the new Airtable setup?

@philsturgeon
Copy link
Collaborator

Sorry or the delays here too! Agh.

This project is finally getting its new home, merged with climatechoice over here impactmakers/climatechoice#161

The airtable source and directory approach is simply being moved over there, so this will still need to be done, just once the dust has settled of that PR. Could you subscribe to the PR and when its done try and help with a country selector?

@philsturgeon
Copy link
Collaborator

Btw I'll try and get the read-only version of the API up so people dont need EVN vars soon.

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.

2 participants