Closed adamhock closed 2 years ago
HeaderBreadcrumbsSplitButton
has been updated and changed to HeaderSplitButton
The majority of the HeaderSplitButton
component is either code taken from SplitButton
or hard-coded HTML to match the DOM structure used in the CSS PR.
The plan was to create two new components, HeaderBasicButton
and HeaderDropdownButton
. Both HeaderDropdownButton
and HeaderSplitButton
would use HeaderBasicButton
in their implementations. HeaderBasicButton
is needed to replace class names that aren't given in the Button
component
iui-button iui-borderless
would be replaced with iui-header-breadcrumb-button
iui-button-label
would be replaced with iui-header-breadcrumb-text
iui-button-icon
would be replaced with iui-header-breadcrumb-button-dropdown-icon
@veekeys, @FlyersPh9, and I were discussing an alternative that would pass a data attribute data-type="header-breadcrumb"
into the Button
component which would give the breadcrumb styling for iui-button-label
and iui-button-icon
. This way, we wouldn't need three different button components just for Header
.
What are your thoughts on this? @mayank99 @bentleyvk
I do not think there is any reason to force the usage of <Button>
since this use case is so different. Plus it would create an unnecessary mess in that component.
But if we are creating new components, I think we should try to think a little bit ahead about the ideal API we want to achieve. This might require rethinking the current API and deprecating parts of it.
Some things to think about:
aria-current
on the <li>
or aria-label
on the <button>
which wraps the caret icon in split button.<ol>
instead of <ul>
.With that in mind, we probably want to go with a composition approach. A rough example of what the usage could look like:
<Header>
<Header.Logo>...</Header.Logo>
<Header.Breadcrumbs>
<Header.BreadcrumbItem>
<Header.Button>...</Header.Button>
</Header.BreadcrumbItem>
<Header.BreadcrumbItem>
<Header.SplitButtonWrapper>
<Header.Button>...</Header.Button>
<Header.SplitMenu>...</Header.SplitMenu>
</Header.SplitButtonWrapper>
</Header.BreadcrumbItem>
<Header.BreadcrumbItem>
<Header.DropdownButton>...</Header.DropdownButton>
</Header.BreadcrumbItem>
</Header.Breadcrumbs>
</Header>
Even if we do not always require the user to use every single subcomponent, I think it would useful to provide them just for the flexibility.
Our current HeaderButton
is not very flexible, and as a result we are seeing less-than-ideal usage out in the wild. For instance, the buttons in developer.bentley.com are wrapped by <a>
because we don't provide a way to change the rendered element. This is outright invalid, inaccessible DOM structure!
So I suggest we start creating these subcomponents now, and see if we can keep backwards compatibility with the old HeaderButton
API. The current iteration of HeaderSplitButton
in this PR is a step in the right direction but it still feels like it's doing too much and is therefore not very flexible. The <li>
should definitely be part of the button component, and I think the two buttons should also be separate components.
I thought we will create new header sub-components from the ground up, without reusing Button with all ugly overrides.
@bentleyvk what ugly overrides you have in mind except css (which is an issue of itwinui-css and button implementation there) ?
I see it opposite. I dont find it nice to have copied code from our buttons, just to have different classes.
I think if me manage to fix css, so that we could achieve any custom button in other components without huge css issues, I see it as an advantage in react.
We still deliver those as separate components, but inner implementation just would use our Button
with data-attribute setting applied (for the css).
So we do not need to copy all the logic and props, cause all of those need to be in header button too (i.e. anchor support).
Code is clean, support is easier and I am not sure I see some huge issues with that.
Yeah, I was talking about CSS, there is about 200 lines of overrides. If the look is so different is it even worth to reuse button? It just increases complexity.
This branch was created to test @FlyersPh9 's header refactor in React
I used the
Button
component to create the three different types of breadcrumb buttons (default, dropdown, and split) which means the following.iui-button .iui-borderless
class names are applied to these buttons which overrides some properties fromiui-header-breadcrumb-button
.iui-button-label
class name in theButton
component toiui-header-breadcrumb-button-text
unless changes are made to theButton
component.iui-header-breadcrumb-button-dropdown-icon
is not applied to the caret svg for the dropdown buttonBreadcrumbButton
component which acts similar toButton
but includes these changes to the class names of the button, label, and dropdown icon?A new component
HeaderBreadcrumbsSplitButton
was created to handle the dropdown side of the split button.When linking this branch to Jon's CSS, I came across some errors when building due to renaming of some CSS files. Any changes shown outside of the
Header
component and its subcomponents are just me commenting out these imports.