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

Use SingleSelect #15990

Merged
merged 9 commits into from Sep 4, 2020
Merged

Use SingleSelect #15990

merged 9 commits into from Sep 4, 2020

Conversation

karlijnbok
Copy link
Contributor

@karlijnbok karlijnbok commented Sep 3, 2020

Context

After some issues arose due to refactoring of the css, it was decided that the select2 implementation in the SEMrushCountrySelector component should be replaced with a react-select component from the monorepo (see javascript#851 for the adjustments made in the monorepo).

IMPORTANT: the styling of the SEMrushCountrySelector component is not completely correct yet (i.e., the placement of the button next to the Select, it is currently still below the Select). An issue has already been added to the board of Team Frontend.

Summary

This PR can be summarized in the following changelog entry:

  • Uses react-select instead of select2 for the SEMrushCountrySelector component.

Relevant technical choices:

Test instructions

This PR can be tested by following these steps:

  • Checkout P3-144-adjust-multiselect on wordpress-seo and the monorepo
  • Run yarn link-monorepo and build free
  • Edit a post and delete the errors in the console as they are not related
  • inster a ckeyphrase and click the "Get related keyphrases button"
  • Make sure the styling of the select is correct
  • Make sure the search function works
  • Make sure the countryCode value in the store is updated when you change the country
  • check the console, see that a warning is reaised but it depends on the internal implementation of the SingleSelect component so it's not due to our PR (if you open the Advanced accordion item below, which uses the ReactSelect as well, you'll see the same warning):
Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended and may indicate bugs in your code. See https://fb.me/react-async-component-lifecycle-hooks for details.
* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state
Please update the following components: AutosizeInput, Select
  • Check anything else you can think of :)
  • run yarn test to see that all the tests are passing (currently there are two console.error calls in another test that passes anyway, so ignore them)
  • Beware that the styling has yet to be fixed (the button goes below the select, and if you use the searching in the select there is a blueish border on focus of the internal text input)

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Fixes [P3-144]

@karlijnbok karlijnbok added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Sep 3, 2020
@karlijnbok karlijnbok added this to the SEMrush milestone Sep 3, 2020
Copy link
Contributor

@JesserH JesserH left a comment

Choose a reason for hiding this comment

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

CR ✅

@JesserH
Copy link
Contributor

JesserH commented Sep 4, 2020

Acceptance ✅

@JesserH JesserH merged commit 314c309 into feature/semrush Sep 4, 2020
@JesserH JesserH deleted the P3-144-adjust-multiselect branch September 4, 2020 12:23
@enricobattocchi enricobattocchi modified the milestones: SEMrush, 15.1 Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants