newscorp-ghfb / NewsKit

The NewsKit Design system
https://newskit.co.uk
Other
130 stars 14 forks source link

Update Tabs component scenarios - Storybook #724

Closed jps closed 1 year ago

jps commented 1 year ago

Description

Updates to the Tabs component scenarios- Storybook

Tasks/Steps

Notes (main differences to what we have currently live versus the designs)

Acceptance/Testing Criteria

Given I am in Storybook

When I view the Tabs component pages

Then the pages will be updated as per the designs

And the pages will be within 1024x768 for Applitools snapshots.

Business Benefit/Value

Consistent Tabs component examples for consumers, aligning to the NK branding.

JohnTParsons commented 1 year ago

@GeriReid @nathanparris Tabs updates are now ready for review. Please leave any comments on this ticket.

I've added a Default story (which wasn't in Figma) as I think it is good practice to always have one of those (making linking from docs to stories more consistent and avoids some e2e test issues that can arise when no default story).

Storybook for Tabs is here: http://ncu-newskit-docs-pr.s3-website-eu-west-1.amazonaws.com/743-tag-storybook/storybook/?path=/docs/components-tabs--story-tabs-default

Code for tabs.stories.tsx is here https://github.com/newscorp-ghfb/NewsKit/pull/789/files#diff-c032da3fa85d47d761ad405952572b9fde5a6b30e5db73cc236465d422588020

You will see that I've put Tabs code in same PR branch as Tag code. This is because e2e tests for Tag were failing, saying there was an a11y issues with Tabs for some reason, even though I had not changed that! Putting full Tabs update in the branch made that issue go away.

GeriReid commented 1 year ago

This looks great @JohnTParsons, mega ticket! Thanks for adding the default story and changing left/right to start/end. A couple of minor changes please:

Scroll overrides Please add a space060 underneath the second example image

Fixed tab indicator percentage size For consistency remove 'Tabs' from Horizontal and Vertical labels image

Custom tab bar track and indicator weight & Custom tab bar indicator transition: Lowercase 'Tab four'. Is there a need for these scenarios to have four tabs, rather than three? If not, fine to omit.

Transitions Could you add a larger padding between examples? Suggest sizing080. The labels and text run together, makes it hard to read.

Rest looks good. Let us know if you spot anything else @nathanparris

JohnTParsons commented 1 year ago

@GeriReid I had to squeeze the Transitions together or they wouldn't fit on a PaginationPage, which I presume has a max-height to help Cypress tests?

GeriReid commented 1 year ago

@JohnTParsons fair enough - ignore me then!

nathanparris commented 1 year ago

Hi @JohnTParsons just a few small points from me

All stories A lot of the stories are not using the default size Medium. Can you remove the small prop from these.

Scroll overrides These are showing with a scroll bar

Icon only Make all the icons IconOutlinedStarOutline

Transitions Second tab to be labeled "Two transition presets" (capital T) Can we also remove the stylePresets from these scenarios - they don't make sense for the transition story

JohnTParsons commented 1 year ago

@GeriReid You will have seen earlier than merging main into feature branch fixed the Transitions height issue.

@nathanparris I have made your requested changes, including removing style presets from Transitions. I then had to make the transition overrides much slower in order to be able to see the difference that changing the transitionPreset(s) made.

GeriReid commented 1 year ago

Thanks @JohnTParsons, happy with my edits.

A couple of Nathan's I can't see updated:

Rest looks good.

JohnTParsons commented 1 year ago

@GeriReid Have pushed those changes. From "Controlled index selection stories", I've removed the icon completely, as it was a hangover from the old code, not part of new Figma design.

nathanparris commented 1 year ago

@JohnTParsons One extra thing I noticed but the rest looks good. Thanks

Icon only - vertical Remove distribution grow, that is causing the tabs to change size when changed

JohnTParsons commented 1 year ago

Distribution grow removed