[PANGOO-2516][BpkNavigationTabGroup] Add Navigation-tab-group component #3599
Conversation
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
Browser supportIf this is a visual change, make sure you've tested it in multiple browsers. |
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
kerrie625
changed the title
| const flightTextElement = screen.getByText('Flights'); | ||
| const flightLink = flightTextElement.closest('a'); | ||
|
|
||
| expect(flightLink).toHaveClass('bpk-navigation-tab-wrap--CanvasDefault'); |
There was a problem hiding this comment.
Shouldn't this be
| expect(flightLink).toHaveClass('bpk-navigation-tab-wrap--CanvasDefault'); | |
| expect(flightLink).toHaveClass('bpk-navigation-tab-wrap--canvas-default'); |
| @include borders.bpk-border-sm(tokens.$bpk-text-disabled-on-dark-day); | ||
|
|
||
| &:hover { | ||
| background-color: tokens.$bpk-core-primary-night; |
There was a problem hiding this comment.
This wouldn't be this night token and would be a private token :)
If they aren't created in Backpack foundations we will need to create them
There was a problem hiding this comment.
I noticed Dark Sky 85 is only exist in Primitive colours . Do you mean I must add this color to Product colours by myself?
There was a problem hiding this comment.
Found bpk-private-navigation-tab-hover-day already have been added in bpk-foundations-web by this PR ,so use it in Navigation tab group this commit
| } | ||
|
|
||
| &--canvas-default { | ||
| background-color: tokens.$bpk-canvas-day; |
There was a problem hiding this comment.
We might need to check this with design because I don't think this is to have a fixed background colour and is just an 'outline'
There was a problem hiding this comment.
Thank your clarification, I got it wrong before. As button have default background-color ,so set background-color as transparent in commit
| @include borders.bpk-border-sm(tokens.$bpk-core-accent-day); | ||
|
|
||
| &:hover { | ||
| background-color: tokens.$bpk-core-accent-day; |
There was a problem hiding this comment.
This is selected button hover color . It will change to white if not add this background-color.
| background-color: tokens.$bpk-canvas-day; | ||
| color: tokens.$bpk-text-primary-day; | ||
|
|
||
| @include borders.bpk-border-sm(tokens.$bpk-text-disabled-day); |
There was a problem hiding this comment.
This token should be
| @include borders.bpk-border-sm(tokens.$bpk-text-disabled-day); | |
| @include borders.bpk-border-sm(tokens.$bpk-line-day); |
| className={tabStyling} | ||
| href={tab.href} | ||
| onClick={onClick} | ||
| aria-current={selected ? 'page' : undefined} |
There was a problem hiding this comment.
According to the spec if you set aria-current to undefined it will set the value to true and tell the assistive tech that the tab is current: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-current#values
So here I would probably see
| aria-current={selected ? 'page' : undefined} | |
| aria-current={selected ? 'page' : false} |
There was a problem hiding this comment.
Thanks for your suggestion .Updated in this commit
| className={tabStyling} | ||
| type="button" | ||
| onClick={onClick} | ||
| aria-current={selected ? 'page' : undefined} |
| <nav | ||
| className={containerStyling} | ||
| role="navigation" | ||
| aria-label="Tab Navigation" |
There was a problem hiding this comment.
Should this be hard coded like this? What happens when it comes to localisation?
Maybe we need to make this value configurable?
There was a problem hiding this comment.
Add airalabel props in this commit
| {Icon ? ( | ||
| <span | ||
| className={getClassName( | ||
| 'bpk-navigation-tab-icon', | ||
| `bpk-navigation-tab-icon--${type}`, | ||
| selected && `bpk-navigation-tab-icon--${type}-selected`, | ||
| )} | ||
| > | ||
| <Icon /> | ||
| </span> | ||
| ) : null} |
There was a problem hiding this comment.
I think you can simplify this without the if else by changing to
| {Icon ? ( | |
| <span | |
| className={getClassName( | |
| 'bpk-navigation-tab-icon', | |
| `bpk-navigation-tab-icon--${type}`, | |
| selected && `bpk-navigation-tab-icon--${type}-selected`, | |
| )} | |
| > | |
| <Icon /> | |
| </span> | |
| ) : null} | |
| {Icon && ( | |
| <span | |
| className={getClassName( | |
| 'bpk-navigation-tab-icon', | |
| `bpk-navigation-tab-icon--${type}`, | |
| selected && `bpk-navigation-tab-icon--${type}-selected`, | |
| )} | |
| > | |
| <Icon /> | |
| </span> | |
| )} |
There was a problem hiding this comment.
Updated in this commit
| selectedIndex={0} | ||
| />, | ||
| ); | ||
| expect(asFragment()).toMatchSnapshot(); |
There was a problem hiding this comment.
We tend to move away from snapshot testing as they can be unreliable.
So here we could do a check for the tabs rendering, such as checking for each of the tabs
There was a problem hiding this comment.
Remove snapshot testing and tabs element test in this commit
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
|
Visit https://backpack.github.io/storybook-prs/3599 to see this build running in a browser. |
olliecurtis
changed the title
During we are doing unified header’s work, we found there is a new navigation tabs component which doesn’t existed in current Backpack components and we need to create one.
So add the Navigation Tab Group component for web, which align with the app
Figma Link
StoryBook
SurfaceContrast:
Without Icon SurfaceContrast:
CanvasDefault:
Without Icon CanvasDefault:
Remember to include the following changes:
[KOA-123][BpkButton] Updating the colourREADME.md(If you have created a new component)README.md