Skip to content

Conversation

taysea
Copy link
Collaborator

@taysea taysea commented Aug 15, 2022

What does this PR do?

Adds size prop to PageHeader and related theming of theme.pageHeader.size.small, theme.pageHeader.size.medium, theme.pageHeader.size.large which can set title, subtitle, and pad values for each size.

Discussion:

  • Are small, medium, and large the right sizes to support out of the box? Designs we have seen only really necessitate a "large" variant in additional to the default, but felt that "small" could be reasonable also.

Where should the reviewer start?

PageHeader.js / base.js

What testing has been done on this PR?

Storybook and unit test.

How should this be manually tested?

Use "Size" PageHeader storybook example.

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Closes #6273

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Yes. grommet/grommet-site#409

Should this PR be mentioned in the release notes?

Yes. Added size to PageHeader.

Is this change backwards compatible or is it a breaking change?

Backwards compatible.

@taysea taysea requested a review from halocline August 15, 2022 21:57
@taysea taysea changed the title 6273 pageheader size Add size to PageHeader Aug 15, 2022
@taysea taysea requested a review from ericsoderberghp August 15, 2022 22:32
Copy link
Collaborator

@halocline halocline left a comment

Choose a reason for hiding this comment

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

small, medium, large seem reasonable to me. Scaling of the font sizes and padding look good.

Minor naming suggestion.

Will need follow on docs PR.

@ericsoderberghp ericsoderberghp merged commit 8b483ae into grommet:master Aug 16, 2022
@taysea taysea deleted the 6273-pageheader-size branch August 16, 2022 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PageHeader - Add size prop and theme options
3 participants