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

Update product editor CES modal to match new designs #38592

Merged
merged 14 commits into from Jun 12, 2023
Merged

Conversation

joshuatf
Copy link
Contributor

@joshuatf joshuatf commented Jun 2, 2023

Submission Review Guidelines:

Changes proposed in this Pull Request:

Updates the fields and designs of the CES modal.

Screen Shot 2023-06-02 at 1 52 57 PM

Closes #38576 .

How to test the changes in this Pull Request:

Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:

  1. Open your console and enter localStorage.setItem( 'debug', '*tracks*' ); and also check "Preserve log"
  2. Navigate to WooCommerce -> Settings -> Advanced -> Features and enable the product block editor
  3. Navigate to the Add Product page
  4. Click the "more" icon (three vertical dots in the upper right) and click "Use the classic editor"
  5. Note the design matches those in the Figma designs in the issue
  6. Make sure that the submit is disabled until at least one checkbox is selected
  7. Note that an event is shown in your console wcadmin_product_mvp_feedback containing the email, comments, and checked reasons

@joshuatf joshuatf requested a review from a team June 2, 2023 20:59
@joshuatf joshuatf self-assigned this Jun 2, 2023
@github-actions github-actions bot added the package: @woocommerce/customer-effort-score issues related to @woocommerce/customer-effort-score label Jun 2, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Hi @nathanss,

Apart from reviewing the code changes, please make sure to review the testing instructions as well.

You can follow this guide to find out what good testing instructions should look like:
https://github.com/woocommerce/woocommerce/wiki/Writing-high-quality-testing-instructions

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2023

Test Results Summary

Commit SHA: 63605db

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests25900202610m 55s
E2E Tests1950010020516m 27s

To view the full API test report, click here.
To view the full E2E test report, click here.
To view all test reports, visit the WooCommerce Test Reports Dashboard.

Copy link
Contributor

@nathanss nathanss left a comment

Choose a reason for hiding this comment

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

Looking good!

Just left one suggestion and one question. Nice job!

</div>
></Text>
<fieldset className="woocommerce-product-mvp-feedback-modal__reason">
<legend>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you increase the margin bottom here? It is very close to the checkboxes and the margin is bigger in the design

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Updated from $gap-smaller to $gap-small.

}

legend {
font-size: 11px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's a question instead of a suggestion: do we have any standards for font-sizes? 11px seems odd (literally). I see it's the same size of the design.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've used it in a lot of places, but I'm not sure if there is a style guide somewhere for this. It would be really nice to create variables around this (e.g., $font-size-label, $font-size-help, etc). Not sure if something like this already exists on the design side. cc @jarekmorawski

Copy link
Contributor

@nathanss nathanss left a comment

Choose a reason for hiding this comment

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

Looking good!

Just left one suggestion and one question. Nice job!

@joshuatf
Copy link
Contributor Author

joshuatf commented Jun 6, 2023

Updated based on your feedback, @nathanss! Should be ready for another review.

nathanss
nathanss previously approved these changes Jun 6, 2023
@nathanss
Copy link
Contributor

nathanss commented Jun 6, 2023

@joshuatf approved! There are some lint problems. Let me know when you fix them and I can approve again

@joshuatf
Copy link
Contributor Author

joshuatf commented Jun 8, 2023

Hey @nathanss, fixed up those lint errors. This PR is ready for another review when you have time 🙇

@nathanss nathanss self-requested a review June 12, 2023 12:53
@joshuatf joshuatf merged commit 90e4ddb into trunk Jun 12, 2023
18 checks passed
@joshuatf joshuatf deleted the update/38576 branch June 12, 2023 17:58
@github-actions github-actions bot added this to the 7.9.0 milestone Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: @woocommerce/customer-effort-score issues related to @woocommerce/customer-effort-score
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Product Editor Onboarding: Create new CES modal
2 participants