refactor(breadcrumbs): component, make it functional and pluggable #32 #94
Conversation
There was a problem hiding this comment.
@nileshgulia1 Nice one.
Let's wait for David's approval and then merge it.
ichim-david
changed the title
There was a problem hiding this comment.
@nileshgulia1 I like what you did as well, but I don't agree with removing the wrapping segment from the breadcrumbs.
If we look at Volto's implementation it has the breadcrumbs segment as well which can be used in case you want to have a certain background set for it. (https://github.com/plone/volto/blob/master/src/components/theme/Breadcrumbs/Breadcrumbs.jsx#L79)
We don't want to have that dark background set, so we should have a variable for the segment background color to have it transparent or white.
Once you bring back the segment I am fine with the other changes.
There was a problem hiding this comment.
@nileshgulia1 your changes are fine for Breacrumbs.jsx which was left over as a customization from volto-ims-theme.
The problem is the fact that this package is using Breacrumbs.stories.jsx which has a bunch of usability issues which I've tracked in ticket #32
We will see what we should do after ITML fixes the issues that I have added, perhaps another ticket will need to be added.
| vertical | ||
| > | ||
| <Container> | ||
| <Breadcrumb size={'tiny'}> |
There was a problem hiding this comment.
Bring back size passed to breadcrumb, make it so that a prop can change the size of the breadcrumb.
We need tiny which sets size to 12px but other designs might be ok to have simply breadcrumbs which
sets the size to 16px
|
nileshgulia1
force-pushed
the
breadcrumbs
branch
from
02cf14f
to
3c8e1c6
Compare
|
@nileshgulia1 Please don't force push. Try to solve the git problem in other means. |
|
@nileshgulia1 I'm going to merge this pull request as this work is fine. We will see later the implementation of breadcrumbs from storybook when ITML fixes the signaled issues |
No description provided.