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

fix: Revert removal of Menu in Preview #2335

Merged

Conversation

mannycarrera4
Copy link
Contributor

@mannycarrera4 mannycarrera4 commented Sep 12, 2023

Summary

Fixes: #2333

Release Category

Components

Release Note

We're going to continue supporting the Menu in Preview until we implement grouped menu items (with virtualization) for the Menu in Main.


Checklist

For the Reviewer

  • PR title is short and descriptive
  • PR summary describes the change (Fixes/Resolves linked correctly)
  • PR Release Notes describes additional information useful to call out in a release message or removed if not applicable
  • Breaking Changes provides useful information to upgrade to this code or removed if not applicable

Where Should the Reviewer Start?

Areas for Feedback? (optional)

  • Code
  • Documentation
  • Testing
  • Codemods

Testing Manually

Screenshots or GIFs (if applicable)

Thank You Gif (optional)

Preview and the `Menu` in Main.

## Deprecations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think anything needs to go into the upgrade guide since we deprecated it in v8?

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me.

Removing the ## Deprecations heading also makes sense since that section is empty, though this means we should also remove the Deprecations line in the table of contents at the top.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we not deprecating anything in v10?

@mannycarrera4 mannycarrera4 added 10.x ready for review Code is ready for review labels Sep 12, 2023
@cypress
Copy link

cypress bot commented Sep 12, 2023

1 flaky test on run #6136 ↗︎

0 937 2 0 Flakiness 1

Details:

Merge 8c0de25 into 9a26501...
Project: canvas-kit Commit: 98af1e6aa8 ℹ️
Status: Passed Duration: 07:54 💡
Started: Sep 12, 2023 9:15 PM Ended: Sep 12, 2023 9:23 PM
Flakiness  cypress/integration/Autocomplete.spec.ts • 1 flaky test

View Output Video

Test Artifacts
... > when a value is entered > when down arrow key is pressed > when the user presses the enter key > when the use hits the "2" key > should change the filtered results Output Screenshots Video

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@jamesfan
Copy link
Member

I'm referencing #2247 (the original PR to remove the Preview Menu) as I'm reviewing this PR to add the Preview Menu back in. Not all of those changes are being reverted here, which I think is ok, but I wanted to call them out to make sure we're being intentional about it:

  1. We should add cypress/integration/MenuPreview.spec.ts back in.
  2. PR2247 replaced DeprecatedMenuItem/DeprecatedMenuItemProps with StyledMenuItem/MenuItemProps throughout labs-react/combobox and labs-react/search-form. We're not reverting these changes here. I think this is ok since the changes only touch tests, stories, and examples (i.e., we may as well use StyledMenuItem instead of DeprecatedMenuItem internally).
  3. We're adding everything back to preview-react/menu. Makes sense. However, we should add this menu export back to modules/preview-react/index.ts.
  4. There are a few changes in PR2247 to react/menu and react/popup that are not being reverted here, but they look ok to me since the react/menu changes have nothing to do with the Preview Menu and the react/popup changes are only to tests.

@jamesfan jamesfan added the review in progress Code is currently under review label Sep 12, 2023
@jamesfan jamesfan removed the ready for review Code is ready for review label Sep 12, 2023
@mannycarrera4 mannycarrera4 changed the title fix: Support menu preview until we figure out virtualization of groups fix: Revert removal of Menu in Preview Sep 12, 2023
@mannycarrera4 mannycarrera4 added automerge and removed review in progress Code is currently under review labels Sep 12, 2023
@alanbsmith alanbsmith merged commit 06bda48 into Workday:prerelease/major Sep 12, 2023
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix: Still support Menu in Preview in v10 while we sort out virtualization for grouping
4 participants