Skip to content

Conversation

HardikChoudhary24
Copy link
Contributor

What does this PR do?

Fixes #6939

  • Adds level prop to PageHeader.

Where should the reviewer start?

  • Changes are done in three files which are PageHeader.js, index.d.ts, propTypes.js

What testing has been done on this PR?

  • Ran locally on storybook.

How should this be manually tested?

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?

#6939

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

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

  • Yes

Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

This could also use a jest testcase for the new level prop

@HardikChoudhary24
Copy link
Contributor Author

@MikeKingdom. I have added the Level story. Are there any changes required?

Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

Looking good. It has a few lint errors that need to be cleaned up and it still needs a jest test case for level

@MikeKingdom
Copy link
Collaborator

@MikeKingdom. I have added the Level story. Are there any changes required?

Thanks! It needs some lint errors cleaned up plus it still needs a jest test case for the level prop

Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for your contribution!

@HardikChoudhary24
Copy link
Contributor Author

Hey @MikeKingdom I wanted to know if there are any additional changes or adjustments needed for my PR, as it has not been merged yet.

@MikeKingdom MikeKingdom requested a review from jcfilben October 9, 2023 16:21
@MikeKingdom
Copy link
Collaborator

Hey @MikeKingdom I wanted to know if there are any additional changes or adjustments needed for my PR, as it has not been merged yet.

No it looks great. I'm just giving @jcfilben a chance to look at it. She was gone the past few days

Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for contributing!

@jcfilben jcfilben merged commit 6f1347d into grommet:master Oct 9, 2023
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 level prop
3 participants