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(breadcrumb): account for menu direction in breadcrumb overflow caret #9189

Conversation

emyarod
Copy link
Member

@emyarod emyarod commented Jul 12, 2021

Closes #9155

This PR fixes the ComponentToggle (ref #8863) display names to be the original component names (it was not being assigned before) to fix a regression in adding breadcrumb overflow menu caret styles. It also accounts for new overflow menu changes (support for more menu directions ref #7116)

Testing / Reviewing

Confirm the breadcrumb overflow menu caret appears as expected

@emyarod emyarod requested a review from a team as a code owner July 12, 2021 20:41
@emyarod emyarod requested review from tw15egan and jnm2377 July 12, 2021 20:41
@netlify
Copy link

netlify bot commented Jul 12, 2021

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

🔨 Explore the source changes: 482916c

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

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

@emyarod
Copy link
Member Author

emyarod commented Jul 12, 2021

cc @tay1orjones, it looks like the displayName was supposed to be set as FeatureToggle(${name}) but it doesn't seem to be possible to do this within CreateComponentToggle without dynamically setting the forwardRef function param. Currently no display name is set on the return value of CreateComponentToggle so for the time being I am manually setting the display names to FeatureToggle(${name}). Let me know what you think

@netlify
Copy link

netlify bot commented Jul 12, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 482916c

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

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

@emyarod emyarod marked this pull request as draft July 12, 2021 20:56
@netlify
Copy link

netlify bot commented Jul 12, 2021

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

🔨 Explore the source changes: 482916c

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

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

@emyarod emyarod force-pushed the 9155-fix-breadcrumbitem-overflowmenu-caret-positioning branch 2 times, most recently from cd7a846 to e8e2016 Compare July 12, 2021 21:07
@emyarod emyarod marked this pull request as ready for review July 12, 2021 21:10
@emyarod emyarod requested a review from a team as a code owner July 12, 2021 21:10
@emyarod emyarod force-pushed the 9155-fix-breadcrumbitem-overflowmenu-caret-positioning branch from e8e2016 to bbf67be Compare July 12, 2021 21:19
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Hm, this is confusing. React docs include this as an explicit example, though the issue from 2018 mentions that the function param must be named.

The DataTable and TableToolbarMenu snapshots include the ForwardRef and FeatureToggle wrapper in the name, ForwardRef(FeatureToggle(OverflowMenu), which I think is ultimately okay. I saw the same thing in the devtools in the initial implementation.

But I get what you're saying - the public API snapshot doesn't actually show displayName as a property of the exported object.

packages/react/src/internal/ComponentToggle.js Outdated Show resolved Hide resolved
@emyarod emyarod force-pushed the 9155-fix-breadcrumbitem-overflowmenu-caret-positioning branch from bbf67be to 0f12487 Compare July 14, 2021 15:24
@emyarod emyarod requested a review from tay1orjones July 14, 2021 15:25
@emyarod emyarod force-pushed the 9155-fix-breadcrumbitem-overflowmenu-caret-positioning branch from 0f12487 to a5fc937 Compare July 14, 2021 15:26
Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Sweet! 🎉 I'm glad that worked out

@emyarod
Copy link
Member Author

emyarod commented Jul 22, 2021

bump @tw15egan @jnm2377 when you have a chance

Copy link
Collaborator

@tw15egan tw15egan left a comment

Choose a reason for hiding this comment

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

LGTM 👍 ✅

@kodiakhq kodiakhq bot merged commit 388008d into carbon-design-system:main Jul 22, 2021
@emyarod emyarod mentioned this pull request Jul 26, 2021
62 tasks
@emyarod emyarod deleted the 9155-fix-breadcrumbitem-overflowmenu-caret-positioning branch July 26, 2021 16:44
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.

React BreadcrumbItem overflow position and shark fin incorrect
3 participants