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

P3 51 connect redux to component #15756

Merged
merged 8 commits into from
Jul 30, 2020

Conversation

JesserH
Copy link
Contributor

@JesserH JesserH commented Jul 29, 2020

Context

  • In this code we connect the first and following redux actions to the components:
  1. Modal-opened REDUX ACTION, which opens the modal
  2. Modal-dismissed REDUX ACTION, which corresponds to the dismissal of the modal
  3. Change-database REDUX ACTION, which corresponds to changing the country of the database in the dropdown menu, in the related-keyphrases component ( P3-5: Create SEMrush Related-Keyphrases ComponentBLOCKED )
  4. New-request REDUX ACTION, which corresponds to the functionality of using the “Change country“ button in the related-keyphrases component ( P3-5: Create SEMrush Related-Keyphrases ComponentBLOCKED ), after which a new request is made with the new database
  • Furthermore we add some architectural changes to the containers which are discussed with @herregroen and @jcomack

Summary

This PR can be summarized in the following changelog entry:

  • non-user facing

Relevant technical choices:

  • The containers are restructured and split up - The modal actions are dispatched in a separate container from the semrush container

Test instructions

This PR can be tested by following these steps:

  • Checkout this branch and build
  • Make sure to have redux devtools installed
  • Enter a keyphrase and hit the get related keyphrases button
  • Make sure the MODAL_OPEN action gets called from 'yoast-seo/editor' within the redux dev tools and make sure that the expected values appear in the state
  • Ensure the same for MODAL_CLOSE when closing the modal and make sure that the expected values appear in the state
  • Open the modal again and select a country form the select box
  • Ensure that the CHANGE_DATABASE action is called and make sure that the expected values appear in the state
  • Close the modal and reopen and verify that it still contains the chosen country as preset
  • Hit the change country button and verify that the NEW_REQUEST button is hit and make sure that the expected values appear in the state

UI changes

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

Quality assurance

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

This way props can be passed easier to the child components
In a call with @jcomack we discussed the possible solutions of a logical refactor from the containers with the corrosponding actions. We will verify with @herregroen later this day
@JesserH JesserH added the changelog: non-user-facing Needs to be included in the 'Non-userfacing' category in the changelog label Jul 29, 2020
@JesserH JesserH added this to the SEMrush milestone Jul 29, 2020
@JesserH JesserH assigned jcomack and unassigned jcomack Jul 29, 2020
Copy link
Contributor

@jcomack jcomack left a comment

Choose a reason for hiding this comment

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

CR done 👍

@jcomack
Copy link
Contributor

jcomack commented Jul 30, 2020

Accceptance done 👍

@jcomack jcomack merged commit 2a10d12 into feature/semrush Jul 30, 2020
@jcomack jcomack deleted the P3-51-connect-redux-to-component branch July 30, 2020 09:35
@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.

4 participants