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

docs(spec): add breadcrumb spec #3552

Merged
merged 7 commits into from
Aug 3, 2020
Merged

Conversation

khamudom
Copy link
Contributor

@khamudom khamudom commented Jul 24, 2020

Description

The file has specs for fast-breadcrumb and fast-breadcrumb-item

image

Motivation & context

Issue type checklist

  • Chore: A change that does not impact distributed packages.
  • Bug fix: A change that fixes an issue, link to the issue above.
  • New feature: A change that adds functionality.

Is this a breaking change?

  • This change causes current functionality to break.

Adding or modifying component(s) in @microsoft/fast-components checklist

Process & policy checklist

  • I have added tests for my changes.
  • I have tested my changes.
  • I have updated the project documentation to reflect my changes.
  • I have read the CONTRIBUTING documentation and followed the standards for this project.

specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
Copy link
Member

@chrisdholt chrisdholt left a comment

Choose a reason for hiding this comment

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

I think we need to dig into the implementation a bit more and understand the value of breadcrumb item. Aside from being able to easily provide a separator within it, it seems to just wrap the anchor that would serve as the primary visual here. I'd like to see if we can simplify this API a bit.

@EisenbergEffect
Copy link
Contributor

@khamudom and I worked through a lot of options and there was definitely a difficulty in dealing with getting to right structure and incorporating the separator. It seems like the only way to get the separator is to have a wrapper element to render it because there's no way to wrap individual slotted elements.

As I was reading the comments I did come up with a new idea though. Caveat: it's dependent on the new work in fast-element and the proposed new FoundationElement base class. With the new base class, all of our components would have a template property to allow for dynamically swapping the template of an element. With that in place, we could do something really interesting...we could have the breadcrumb component grab the template of each of its children, and then create a new template that wraps the current template so that it can apply the configured separator. It can then assign the new composed template back to the anchor instances. What do you all think of that idea?

@chrisdholt
Copy link
Member

@EisenbergEffect I was rolling around something like that in my head but wasn’t sure how to reconcile with composability. I think that would be preferred here and a welcome addition. It’s worth waiting for that work in my opinion.

@EisenbergEffect
Copy link
Contributor

There are a few more tricks to it since the child anchors would not be connected when the breadcrumbs connected callback fires. So, the child templates wouldn't be in place yet to wrap. I've got a couple of solutions in mind to it and will keep thinking about it a bit more. It might be worth introducing "template decorators" as an official concept for things like this and have some standard way of doing it. Need to think about it a bit more.

@radium-v
Copy link
Collaborator

Would css counter styles work for the separators?

@EisenbergEffect
Copy link
Contributor

@radium-v I'm not sure. It probably depends on how flexible we want to make the separator. What if someone has a custom SVG, for example, do we want to support that or just a basic character?

@radium-v
Copy link
Collaborator

@EisenbergEffect my bad, I was thinking about list-style properties which do allow for images. Interestingly, the breadcrumb example linked in the README uses empty ::before pseudo-elements to avoid excessive verbosity with screen readers.

@EisenbergEffect
Copy link
Contributor

Is it possible to apply a list style to something that isn't actually an li? If we give it an aria listitem role, would that styling work? I'd certainly prefer that type of solution if possible.

specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
@khamudom khamudom force-pushed the users/khamu/breadcrumb-specs branch from a1b4b0e to 17795e7 Compare July 29, 2020 16:16
@khamudom
Copy link
Contributor Author

Updated the fast-breadcrumb-item. instead of having an empty slot, added am anchor as a default control. This cleans up the html, it avoids having to add another control inside fast-breadcrumb-item.

@khamudom khamudom force-pushed the users/khamu/breadcrumb-specs branch from 7eaa14e to 2bffe69 Compare July 30, 2020 01:05
specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
specs/breadcrumb.md Outdated Show resolved Hide resolved
@khamudom khamudom force-pushed the users/khamu/breadcrumb-specs branch 2 times, most recently from 0e9bc4d to d7f2c82 Compare July 31, 2020 15:58
@khamudom khamudom force-pushed the users/khamu/breadcrumb-specs branch from d7f2c82 to c2b2d41 Compare July 31, 2020 19:01
@khamudom khamudom force-pushed the users/khamu/breadcrumb-specs branch from c2b2d41 to 51e120c Compare July 31, 2020 23:48
@khamudom khamudom merged commit f9dd0a4 into master Aug 3, 2020
@khamudom khamudom deleted the users/khamu/breadcrumb-specs branch August 3, 2020 16:00
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.

6 participants