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.

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

feat(breadcrumb): add overflow menu support #7085

Merged
merged 9 commits into from
Mar 29, 2021
Merged

feat(breadcrumb): add overflow menu support #7085

merged 9 commits into from
Mar 29, 2021

Conversation

tw15egan
Copy link
Member

@tw15egan tw15egan commented Oct 19, 2020 •
edited
Loading

Closes #7019
Closes #7016

Adds in support for OverflowMenu nested in BreadcrumbItem

Changelog

New

  • Story for the new variant
  • Style overrides to achieve correct visual styles for OverflowMenu inside a BreadcrumbItem

Changed

  • Modified BreadcrumbItem.js to pass down a className to the FloatingMenu placed by the OverflowMenu. This is needed to create the shark fin styles, as the FloatingMenu is placed outside of the entire Breadcrumb component and there was no way to scope the styles.
  • Also passes in custom menuOffset props to align the menu to the middle of the overflow menu

Testing / Reviewing

Verify the OverflowMenu variant renders properly across all browsers

@netlify
Copy link

netlify bot commented Oct 19, 2020

Deploy preview for carbon-elements ready!

Built with commit ac59013

https://deploy-preview-7085--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 19, 2020

Deploy preview for carbon-components-react ready!

Built with commit ac59013

https://deploy-preview-7085--carbon-components-react.netlify.app

@netlify
Copy link

netlify bot commented Oct 19, 2020 •
edited
Loading

Deploy preview for carbon-elements ready!

Built with commit 0945967

https://deploy-preview-7085--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Oct 20, 2020 •
edited
Loading

Deploy preview for carbon-components-react ready!

Built with commit 0945967

https://deploy-preview-7085--carbon-components-react.netlify.app

@tw15egan tw15egan marked this pull request as ready for review October 20, 2020 17:21
@tw15egan tw15egan requested a review from a team as a code owner October 20, 2020 17:21
laurenmrice
laurenmrice requested changes Oct 20, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Can we make these tweaks to the overflow icon and rule:

  • Align the overflow dots to the baseline of the breadcrumb text.
  • Have 2px space between the overflow dots and the rule.
  • Make the rule 12px long so it aligns flush with the overflow dots.

breadcrumb1

There is a break line showing up between the sharkfin triangle and the container of the overflow menu. They should be connected to eachother and not have the break.
breadcrumb2

The sharkfin triangle point needs to align to the middle overflow menu dot. Right now it is a little to the left.
breadcrumb3

@laurenmrice
Copy link
Member

There is no more flashing of color for the breadcrumbs in Firefox for me. 👍🏻

@tw15egan tw15egan force-pushed the breadcrumb-overflow branch 2 times, most recently from 6767c8d to e9493ff Compare October 20, 2020 22:34
@tw15egan
Copy link
Member Author

@laurenmrice updated 👍

laurenmrice
laurenmrice requested changes Oct 21, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Safari specific bugs:

  • The underline is really close to the the overflow menu dots on hover.
  • There is this weird focus happening when you click on an overflow option. I am not sure if that is possible to get rid of?

Oct-21-2020 12-47-13

Everything else is looking great!

laurenmrice
laurenmrice approved these changes Oct 22, 2020
View reviewed changes
Copy link
Member

@laurenmrice laurenmrice left a comment

Choose a reason for hiding this comment

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

Looks wonderful in Safari now! Thank you ! 🎉

@tay1orjones tay1orjones mentioned this pull request Oct 29, 2020
Open
2 tasks
tay1orjones
tay1orjones reviewed Oct 29, 2020
View reviewed changes
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.

@tw15egan could the docs pane be updated to describe this feature/story? The 'show code' source view alone would be quite helpful imo

@alisonjoseph alisonjoseph mentioned this pull request Oct 30, 2020
Closed
38 tasks
@lee-chase
Copy link
Member

Hi, @tw15egan I have this responsive in-progress prototype carbon-design-system/ibm-products#252 and have a couple of questions.

  1. What positions can the overflow go in?
  2. Can there be only overflow?

I have allowed that as I don't know what width my container will be.

Currently only visible here (see controls panel to adjust width)

May arrive here soon

@lee-chase
Copy link
Member

lee-chase commented Dec 3, 2020 •
edited
Loading

Hi, @tw15egan I have this responsive in-progress prototype carbon-design-system/ibm-products#252 and have a couple of questions.

  1. What positions can the overflow go in?
  2. Can there be only overflow?

I have allowed that as I don't know what width my container will be.

Currently only visible here (see controls panel to adjust width)

May arrive here soon

BreadcrumbWithOverflow

@tw15egan
Copy link
Member Author

tw15egan commented Dec 3, 2020

@lee-chase Nice! As far as your questions go:

  1. I believe since it will only contain a list of BreadcrumbItem links, it would be placed between the first and last BreadcrumbItem. I think it could be confusing if the overflow menu appeared first or last, since those breadcrumbs are usually there to help with a user's context of where they are in an app. I'll defer to @laurenmrice though.

  2. Oooo, I see that yours dynamically adds items to the overflow menu. That's a nice feature! This PR was originally just a quick fix to allow proper styling if an OverflowMenu was a child of BreadcrumbItem, but a more fully-featured implementation would be more of a benefit to our users. Wondering if we should port those changes here?

This was referenced Dec 3, 2020
Closed
Closed
@aledavila aledavila mentioned this pull request Jan 25, 2021
Closed
46 tasks
This was referenced Feb 10, 2021
Merged
Closed
@joshblack joshblack removed their request for review February 24, 2021 21:19
Base automatically changed from master to main March 8, 2021 16:35
@laurenmrice
Copy link
Member

Can we merge this in?

@kodiakhq kodiakhq bot merged commit 431e5fc into carbon-design-system:main Mar 29, 2021
@lee-chase lee-chase mentioned this pull request Apr 16, 2021
Merged
@tw15egan tw15egan deleted the breadcrumb-overflow branch March 3, 2022 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emyarod emyarod emyarod left review comments

@tay1orjones tay1orjones tay1orjones approved these changes

@jnm2377 jnm2377 jnm2377 approved these changes

@laurenmrice laurenmrice laurenmrice approved these changes

@aledavila aledavila Awaiting requested review from aledavila

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.

[breadcrumb] needs to have an overflow option [breadcrumb] hover state bug
6 participants
@tw15egan @laurenmrice @lee-chase @tay1orjones @emyarod @jnm2377