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

feat(breadcrumb): draft of breadcrumb web component #8

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mndonx
Copy link

@mndonx mndonx commented Nov 20, 2023

Summary

This PR adds a Breadcrumb menu web component to the UI Kit.

Screenshot 2023-11-19 at 10 51 08 PM

How to review this pull request

  • Run npm run develop
  • Open the Emulsify instance
  • Click on the Breadcrumbs link in the sidebar
  • Ensure that the breadcrumb works as expected.

Copy link
Contributor

@joetower joetower left a comment

Choose a reason for hiding this comment

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

@mndonx This looks good to me, nice work!

My only question is: should we add a current or active state? Maybe a status option or something which we could style to distinguish it as the current path item - which would not be a linked item? In this case, item level 3 would get the current/active state.

@mndonx
Copy link
Author

mndonx commented Dec 18, 2023

@joetower Thanks Joe! I've added contingency for the "current" item so it is not a link and has a special class. That is ready for review.

However, I still have a couple issues to resolve.

  • @cienvaras showed us how to include some common variables in a different file, but I totally forgot to write it down or record it, so I will need a refresher on how it is done since that was a few weeks ago now! I'd like to include that here.
  • The colors are not passing contrast tests, but I don't want to mess with the variables. What is the process for changing the tokens?

@mndonx
Copy link
Author

mndonx commented Jan 15, 2024

@joetower @cienvaras I've updated this PR to have a way to use a reusable class (visually-hidden.) Let me know what you think of the solution (I read about it in the Lit documentation.)

There is definitely still an issue of color contrast. I added an issue in the Emulsify UI Kit to address the contrast issues across the project. I wonder if I should wait on that, or move this forward?

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.

2 participants