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.

Sign up for GitHub

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

Jump to bottom

Fork ChooseAudience component and use it in EditFreeCampaign #303

Merged
merged 5 commits into from
Mar 9, 2021
Merged

Fork ChooseAudience component and use it in EditFreeCampaign #303

merged 5 commits into from
Mar 9, 2021

Conversation

tomalec
Copy link
Member

@tomalec tomalec commented Mar 8, 2021 •
edited
Loading

Changes proposed in this Pull Request:

  • Duplicate ChooseAudience component,
  • remove saving strategy from it
  • move reusable parts to .~/components/free-listings
  • make step header configurable
  • use it in EditFreeCampaign

Screenshots:

image

ChooseAudience-no-save.mp4

Detailed test instructions:

  1. Visit https://gla1.test/wp-admin/admin.php?page=wc-admin&path=%2Fgoogle%2Fedit-free-campaign&programId=123
  2. Make sure step Content "Configure your product listings" step title looks as in Figma

Still missing

Things to be addressed in future PRs:

@tomalec
Move SupportedCountrySelect and ChooseAudience styles to shared compo…
a8678fb 
…nents.

So they could be reused in the edit flow.
@tomalec
Duplicate ChooseAudience component,
e5056bf 
and place it in the shared component folder,
to make it re-usable and shared with edit camapign flow.
Add some code docs to cross-reference duplicates.
@tomalec
Make shared ChooseAudience not auto-save,
d47f757 
use that version in EditCampaign.
@tomalec
Make ChooseAudience's step header configurable.
bd22f0a
@tomalec tomalec self-assigned this Mar 8, 2021
@tomalec
Merge branch 'trunk' into feature/156-edit-choose-audience
bf78388 
Conflicts:
  js/src/components/free-listings/choose-audience/supported-country-select.js -> js/src/components/free-listings/choose-audience/supported-country-select.js
  js/src/setup-mc/setup-stepper/choose-audience/form-content.js
  js/src/components/free-listings/choose-audience/form-content.js
@tomalec tomalec requested a review from a team March 8, 2021 18:00
eason9487
eason9487 approved these changes Mar 9, 2021
View reviewed changes
Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

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

Although there is some duplicated code between two ChooseAudience components, I assume they will have more differences after completing the subsequent implementations. So I think it is OK to continue with the current version and see how they could be extracted from those reuse parts later.

@tomalec tomalec mentioned this pull request Mar 9, 2021
Closed
@tomalec
Copy link
Member Author

tomalec commented Mar 9, 2021

Although there is some duplicated code between two ChooseAudience components, I assume they will have more differences after completing the subsequent implementations. So I think it is OK to continue with the current version and see how they could be extracted from those reuse parts later.

Sorry, I didn't state it here clearer.
Yes, that's the plan. Make this a redundant duplicate now, to clearly leave the current onboarding flow untouched, to avoid regression issue, due to lack of automated tests. Then shape the duplicate into a more reusable one, which will be used now by editing flow. Then in a calmer moment of the project, we could address this technical debt, and make it DRYier also for onboarding.

This PR introduces mostly duplication, to be able to split the work sooner, avoid conflicts, make it easier to review, and assure the original flow is safe. Then the changes to the duplicated shared component will follow.

@tomalec tomalec merged commit 384561e into trunk Mar 9, 2021
@tomalec tomalec deleted the feature/156-edit-choose-audience branch March 9, 2021 20:25
@eason9487
Copy link
Member

Got it. Thanks for the additional information!

@ecgan
Copy link
Member

ecgan commented Mar 11, 2021

@eason9487 , a little bit more context on the code duplication and future plan, see #288. That issue is about Setup MC Step 3 shipping rates and shipping times stuff, but the concept and idea is related to @tomalec's PR here. The reason is mainly due to having auto-save in setup onboarding flow and manual save in edit flow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eason9487 eason9487 eason9487 approved these changes

Assignees

@tomalec tomalec

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomalec @eason9487 @ecgan