Skip to content

fix(ComposedModal): Address several problems with selectorsFloatingMenus prop on ComposedModal#10003

Merged
kodiakhq[bot] merged 9 commits into
carbon-design-system:mainfrom
dcwarwick:issue10000
Nov 8, 2021
Merged

fix(ComposedModal): Address several problems with selectorsFloatingMenus prop on ComposedModal#10003
kodiakhq[bot] merged 9 commits into
carbon-design-system:mainfrom
dcwarwick:issue10000

Conversation

@dcwarwick

@dcwarwick dcwarwick commented Oct 31, 2021

Copy link
Copy Markdown
Contributor

Closes #10000

Changelog

Changed

  • packages/react/src/components/ComposedModal/ComposedModal.js -- fixed proptype, and remove prop from ...others that are passed through to the DOM.
  • packages/react/src/internal/wrapFocus.js -- default Carbon CSS selectors for floating menus/popus are added to prop value rather than replaced by it.

Testing / Reviewing

Build, tests, and storybook can help verify that this PR does not break existing usage. In particular, a story or test case with a menu/tooltip/date-picker inside a composed modal would be useful to verify that focus handling continues to work as expected.

I will also apply the changes locally to a build of carbon-design-system/ibm-products#1360 and verify that the issues that were problematic with that fix are resolved by these changes.

@dcwarwick dcwarwick requested a review from a team as a code owner October 31, 2021 16:15
@netlify

netlify Bot commented Oct 31, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: a6695b9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/618952f64004980008636a4b

😎 Browse the preview: https://deploy-preview-10003--carbon-react-next.netlify.app

@netlify

netlify Bot commented Oct 31, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: a6695b9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/618952f6712cfb0008fb1998

😎 Browse the preview: https://deploy-preview-10003--carbon-elements.netlify.app

@netlify

netlify Bot commented Oct 31, 2021 •
edited
Loading

Copy link
Copy Markdown

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: a6695b9

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/618952f673cf450008dc5e2f

😎 Browse the preview: https://deploy-preview-10003--carbon-components-react.netlify.app

@dcwarwick dcwarwick requested a review from a team as a code owner November 1, 2021 01:00
joshblack
joshblack reviewed Nov 1, 2021
View reviewed changes
Comment thread packages/react/src/components/ComposedModal/ComposedModal.js
tay1orjones
tay1orjones approved these changes Nov 2, 2021
View reviewed changes

@tay1orjones tay1orjones left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These make sense to me - thanks for including so much detail in the original issue!

How fun that you got to open and close the 10,000th issue on the repository! 🏅 🎉

@kodiakhq kodiakhq Bot merged commit a8a128c into carbon-design-system:main Nov 8, 2021
@tay1orjones tay1orjones mentioned this pull request Jul 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tay1orjones tay1orjones tay1orjones approved these changes

+1 more reviewer

@joshblack joshblack joshblack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status: ready to merge 🎉

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug]: Several problems with selectorsFloatingMenus prop on ComposedModal

3 participants

@dcwarwick @tay1orjones @joshblack