newscorp-ghfb / NewsKit

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

(Story) - PPDSC-2804/Update Modal component scenarios - Storybook #777

Closed RashikaNewsUK closed 1 year ago

RashikaNewsUK commented 1 year ago

Updates to the Modal component scenarios- Storybook

Tasks/Steps

Using the designs update the storybook pages for the Modal component.

The designs include what should be on each page, and the title, description, and link to the documentation page on the site for the component.

Note the name of the pages also should be updated to match what we have in the designs.

Stories need to be renamed - but not moved at this point in time.

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

We do not need the ‘docs’ page in storybook for these examples.

Combine ‘modal-layouts-only’ examples under ‘Modal’, so we have them under one title.

We need modal-layouts-only for percy testing.

Create a follow-up ticket to investigate better ways to handle percy tests in storybook that do not need to be customer facing

Acceptance/Testing Criteria

Given I am in Storybook

When I view the Modal 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 Modal component examples for consumers, aligning to the NK branding.

GeriReid commented 1 year ago

Hi @jannuk59, this looks good. A couple of edits please:

On the dark theme, the modal is almost the same colour as the background so it's hard to see the panel. Would it be possible to change a couple of values in the dark theme for this story to give better contrast?

Thanks

GeriReid commented 1 year ago

@nathanparris PR here if you want to run through.

nathanparris commented 1 year ago

@jannuk59 for the select and nested modal stories, can the height of the modal increase so scroll bars are not visible.

mutebg commented 1 year ago

@GeriReid Janani asked me to check these colours and how we can implement them:

The background for the storybook is always "interfaecBackground" it does not matter the theme, we probably can change it only for the modal but I do not suggest that since it is a kind of exclusion that we need to maintain in the future.

I checked the Drawer and saw that it looks good, the difference is that the Drawer uses interface010 and the modal uses interfaceBackground. So in the case of modal, we have a background with interfaceBackground and modal with interfaceBackground, isn't it better to change the modal to use interface010 ? and yes, that is a breaking change.

Image

GeriReid commented 1 year ago

thanks @mutebg - let me check with the design team.

GeriReid commented 1 year ago

@mutebg caught up with @nathanparris - the modal component has a background colour of interface010 in Figma. Can we change the background of the modal to interface010 in the code to align? The poor contrast currently makes it hard to use in the dark theme.

Do we need a separate ticket for this?

mutebg commented 1 year ago

I prefer a separate ticket. Do we think this is a bug or do we need to introduce it as a breaking change?

GeriReid commented 1 year ago

I'll make a new ticket. What is best practice from your side on how to handle it?

mutebg commented 1 year ago

What @jps said before is something like, if we think this is a bug we can push it as non-breaking change; if think this is a change it needs to be a breaking one.

Do you know if any team is using the dark theme? IMO this can go as a "bug"

GeriReid commented 1 year ago

@nathanparris do you know if anyone other than us is using the modal in a dark theme? If not, could go as a bug.

nathanparris commented 1 year ago

I'm not sure if anyone is using the modal in a dark theme, but will do a quick pass of all the themes to check the default colours - making sure interfaceBackground and interface010 are the same (probably white in all cases but need to check)

jannuk59 commented 1 year ago

@GeriReid and @nathanparris is this ticket is good to go with Design review? Shall I move it to peer review?

GeriReid commented 1 year ago

Hi @jannuk59, I think the Storybook examples are fine - we'll raise a new ticket if we decided to change the component. Okay with you @nathanparris ?

nathanparris commented 1 year ago

@jannuk59 @GeriReid All OK with me