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): add proposal #165

Merged
merged 30 commits into from
Mar 14, 2022

Conversation

assuncaocharles
Copy link
Contributor

Adds Breadcrumb proposal

@assuncaocharles
Copy link
Contributor Author

Hey @gregwhitworth

Have a look in this one when you can :)

Co-authored-by: Roman Sudarikov <pompomon@users.noreply.github.com>
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
@gregwhitworth
Copy link
Member

@assuncaocharles I left a comment, ultimately I think it'd be cool to jump on a call for 30 minutes to work through some of these and get on the same page. To help land this. DM me on Twitter or email me so we can set this up.

research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
Copy link
Member

@gregwhitworth gregwhitworth left a comment

Choose a reason for hiding this comment

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

Not sure how to represent like that in the anatomy. How it would be in the anatomy @una @gregwhitworth @chrisdholt ?

Technically the ::marker psuedo element is there for any li so we don't have to represent it. Although I get your reasoning for including it, what you have represented is fine with me although I'd prefer for it to not be side by side, or to make the psuedo element size to min-content. This makes the psuedo element appear much larger than that of item.

Also, the box itself of the marker comes before the item itself so it should be placed first.

Screen Shot 2020-11-25 at 1 54 11 PM

@assuncaocharles
Copy link
Contributor Author

Not sure how to represent like that in the anatomy. How it would be in the anatomy @una @gregwhitworth @chrisdholt ?

Technically the ::marker psuedo element is there for any li so we don't have to represent it. Although I get your reasoning for including it, what you have represented is fine with me although I'd prefer for it to not be side by side, or to make the psuedo element size to min-content. This makes the psuedo element appear much larger than that of item.

Also, the box itself of the marker comes before the item itself so it should be placed first.

Screen Shot 2020-11-25 at 1 54 11 PM

@gregwhitworth In the image bellow ::marker is inside li as a pseudo-element would be.

98810991-dceb3700-23fe-11eb-8fa6-44f10cfbea66

@kant2002
Copy link
Contributor

I notice that SAP Fiory provide some interesting concept for Breadcrumb.
image
I think this concept valuable for large projects, where breadcrumbs can be rather large in length

My understanding that this cannot be represented in current breadcrumb proposal. So question, should such advanced cases like this be part of Open UI?

@kant2002
Copy link
Contributor

The Carbon Design System from IBM suggest different way for collapse of breadcrumbs

image

from there

@una
Copy link
Collaborator

una commented Dec 16, 2020

It seems like there are open issues around a breadcrumb dropdown functionality. That seems like a (very useful) combination of components, rather than default base functionality. Would you all be okay with merging this PR and taking that discussion into a new issue?

@una
Copy link
Collaborator

una commented Dec 16, 2020

@assuncaocharles did you add ::marker? I don't see it in the PR. Would love to merge it in

@assuncaocharles
Copy link
Contributor Author

@una i did not, I'm not sure how to express that in the anatomy as I said before

@yoavweiss yoavweiss changed the base branch from master to main March 5, 2021 21:45
@yoavweiss yoavweiss requested a review from una as a code owner March 5, 2021 21:45
@gregwhitworth
Copy link
Member

@assuncaocharles this one looks correct to me, granted it wouldn't have the divider part on it: #165 (comment)

Finally, and this is what I'd like @chrisdholt and @una to weigh in on - do we really need to add ::marker as it is already a standardized pseudo element of a list-item?

@chrisdholt
Copy link
Collaborator

@assuncaocharles this one looks correct to me, granted it wouldn't have the divider part on it: #165 (comment)

Finally, and this is what I'd like @chrisdholt and @una to weigh in on - do we really need to add ::marker as it is already a standardized pseudo element of a list-item?

You can create the same UIA representation using ARIA and while I know that's not completely ideal, it is valid and ::marker wouldn't work there. I don't know that we need to make that the rule though, more saying that both can be accurate representations - this really comes down to nuance in my mind.

@gregwhitworth
Copy link
Member

@assuncaocharles can you do a final pass looking at @chrisdholt commit recommendations so that we can do an initial landing of this PR

@andrico1234
Copy link
Collaborator

Hi folks (@gregwhitworth @una @chrisdholt @assuncaocharles )

I've updated this PR to include Chris' suggested changes. Are you happy for me to merge this in?

Copy link
Collaborator

@scottaohara scottaohara left a comment

Choose a reason for hiding this comment

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

things to update / considerations to make

research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Outdated Show resolved Hide resolved
research/src/pages/breadcrumb.proposal.mdx Show resolved Hide resolved
andrico1234 and others added 6 commits March 7, 2022 21:35
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
Co-authored-by: Scott O'Hara <scottaohara@users.noreply.github.com>
@andrico1234
Copy link
Collaborator

Thanks for taking the time to leave comments @scottaohara , I've merged in the typo corrections and other suggested changes.

As for the rest of the comments, I'm not sure how to best approach them. I'll be happy to make notes within the proposal about these unresolved questions, e.g. the correct markup for the spacer, to get this (20 month old 😱) PR merged in. Whoever decides to champion this proposal in the future can resolve the comments.

@andrico1234 andrico1234 dismissed gregwhitworth’s stale review March 11, 2022 21:29

I believe this was addressed in a previous commit a while back. Happy to make additional changes if they're still outstanding

@andrico1234 andrico1234 merged commit 945d983 into openui:main Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants