Skip to content

Add name to select control popover slots #35353

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
Merged
louwie17 merged 4 commits into from
Oct 27, 2022
Merged

Add name to select control popover slots #35353

louwie17 merged 4 commits into from
Oct 27, 2022

Conversation

@louwie17
Copy link
Contributor

@louwie17 louwie17 commented Oct 27, 2022

All Submissions:

Changes proposed in this Pull Request:

Adds a name to the Popover and PopoverSlot so the Popover.Slot doesn't get used for all the popovers within the page.

  • This PR is a very minor change/addition and does not require testing instructions (if checked you can ignore/remove the next section).

How to test the changes in this Pull Request:

  1. Load this branch, build it, and make sure you have the new-product-management-experience feature flag enabled (you can do so using the latest version of the Beta tester within the mono repo).
  2. Go to Products > Add new (MVP) and scroll down to the attributes section
  3. Click Add first attribute and make sure the select control dropdown still shows correctly.

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you created a changelog file for each project being changed, ie pnpm --filter=<project> changelog add?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@louwie17 louwie17 requested a review from a team October 27, 2022 12:31
@github-actions github-actions bot added focus: react admin package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin. labels Oct 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2022 •
edited
Loading

Test Results Summary

Commit SHA: e8865e3

Test 🧪Passed ✅Failed 🚨Broken 🚧Skipped ⏭️Unknown ❔Total 📊Duration ⏱️
API Tests23200302350m 48s
E2E Tests186006019217m 24s
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.

mattsherman
mattsherman suggested changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@mattsherman mattsherman left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works well. Nice change!

As mentioned in Slack, I had a reservation about the use of __unstableSlotName, but it doesn't really look too unstable from what you mentioned:

Only change I suggested was updating the changelog to be a bit more descriptive.

packages/js/components/changelog/fix-select-control-popover-slots Outdated
Significance: patch
Type: fix

Add name to exported popover slot so that it is not used for any popover items.
Copy link
Contributor

@mattsherman mattsherman Oct 27, 2022

Choose a reason for hiding this comment

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

"not used for any popover items" should probably be "not used for all popover items"

Copy link
Contributor

@mattsherman mattsherman Oct 27, 2022

Choose a reason for hiding this comment

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

Oh yeah... would probably be good to mention the context of what component we are talking about.

Copy link
Contributor Author

@louwie17 louwie17 Oct 27, 2022

Choose a reason for hiding this comment

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

🤣 yeah that is very ambiguous

mattsherman
mattsherman suggested changes Oct 27, 2022
View reviewed changes
packages/js/components/changelog/fix-select-control-popover-slots Outdated
Type: fix

Add name to exported popover slot so that it is not used for any popover items.
Add name to exported popover slot used to display SelectControl Menu, so it is only used for SelectControl menu's.
Copy link
Contributor

@mattsherman mattsherman Oct 27, 2022

Choose a reason for hiding this comment

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

Ugh, I hate to be picky... but, it should be "menus", not "menu's"

mattsherman
mattsherman approved these changes Oct 27, 2022
View reviewed changes
Copy link
Contributor

@mattsherman mattsherman left a comment

Choose a reason for hiding this comment

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

Thanks for making the changelog change. Looks good!

@louwie17 louwie17 merged commit 04d6e88 into trunk Oct 27, 2022
@louwie17 louwie17 deleted the fix/select-control-popover-slots branch October 27, 2022 15:52
@github-actions github-actions bot added this to the 7.2.0 milestone Oct 27, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Oct 27, 2022

Hi @louwie17, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the release: add testing instructions label

@github-actions github-actions bot mentioned this pull request Dec 15, 2022
Closed
@github-actions github-actions bot mentioned this pull request Jan 25, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@mattsherman mattsherman mattsherman approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package: @woocommerce/components issues related to @woocommerce/components plugin: woocommerce Issues related to the WooCommerce Core plugin.

Projects

None yet

Milestone

7.2.0

Development

Successfully merging this pull request may close these issues.

3 participants

@louwie17 @mattsherman