Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(723): Standfirst Storybook Enhancement #881

Merged
merged 10 commits into from
May 16, 2023

Conversation

jannuk59
Copy link
Contributor

@jannuk59 jannuk59 commented May 8, 2023

closes #723

What

  1. Background - why this is needed
  2. What did you do
  3. What does the reviewers should expect

I have done:

  • Written unit tests against changes
  • Written functional tests against the component and/or NewsKit site
  • Updated relevant documentation

I have tested manually:

  • The feature's functionality is working as expected on Chrome, Firefox, Safari and Edge
  • The screen reader reads and flows through the elements as expected.
  • There are no new errors in the browser console coming from this PR.
  • When visual test is not added, it renders correctly on different browsers and mobile viewports (Safari, Firefox, small mobile viewport, tablet)
  • The Playground feature is working as expected

Before:

After:

Who should review this PR:

How to test:

@github-actions github-actions bot added the docs Improvements or additions to documentation label May 8, 2023
@pp-serviceaccount
Copy link
Collaborator

@jannuk59 jannuk59 added ready for design review Please assist in getting this reviewed draft This is a draft PR and not intended for formal review yet ready for review Please assist in getting this reviewed and removed ready for design review Please assist in getting this reviewed labels May 8, 2023
@mutebg mutebg removed the draft This is a draft PR and not intended for formal review yet label May 10, 2023
overrides={{
styledText: {
stylePreset: 'standfirstCustom',
typographyPreset: 'utilitySubheading020',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is default I am expecting it not to have any overrides

overrides={{
styledText: {
typographyPreset: 'utilitySubheading020',
stylePreset: 'standfirstCustom',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can use directly inkBrand010 as style-preset, not need to create a custom one, newskit automatically creates style presets for all colors tokens which start with ink

Suggested change
stylePreset: 'standfirstCustom',
stylePreset: 'inkBrand010',

@jannuk59 jannuk59 marked this pull request as ready for review May 11, 2023 06:52
Comment on lines 10 to 17
const CONTENT = (
<TextBlock typographyPreset="utilitySubheading020">
NewsKit provides components, guidelines and standards to enable digital
product teams to create high-quality, consistent products quickly. NewsKit
is built on modular design principles and backed by best practice guidance
for design and development.
</TextBlock>
);
Copy link
Contributor

@mutebg mutebg May 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use text-block with typographyPreset to pass it down to the standfirst component?
Is that required by design? if so this needs to change, that's not how we suggest this component to be used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mutebg as per design its typographyPreset="utilitySubheading020" by default we have

typographyPreset: {
        xs: 'editorialSubheadline010',
        lg: 'editorialSubheadline020',
},

So I have added text-block component to update the typographyPreset

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the designs are wrong, the default version is without any props and overrides

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mutebg @jannuk59 The standfirst already has typography presets in the defaults, so there shouldn't be any need to add these to a content text block.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nathanparris this exactly what I meant. thanks for clarifying it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done @mutebg @nathanparris. By default will be having

typographyPreset: {
        xs: 'editorialSubheadline010',
        lg: 'editorialSubheadline020',
},

>
{bodyString}
</Standfirst>
<Standfirst as="h2">{CONTENT}</Standfirst>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this is default story, I would remove as="h2" prop

JohnTParsons
JohnTParsons previously approved these changes May 15, 2023
@jannuk59 jannuk59 merged commit 6bd5bd6 into main May 16, 2023
@jannuk59 jannuk59 deleted the docs/723-standfirst-storybook branch May 16, 2023 08:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation ready for review Please assist in getting this reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Standfirst component scenarios - Storybook
5 participants