Skip to content

Conversation

hwdsjtu97
Copy link
Contributor

@hwdsjtu97 hwdsjtu97 commented Feb 18, 2023

What does this PR do?

Add support for using between border in border array props

Where should the reviewer start?

Box.js

What testing has been done on this PR?

yarn test passed

How should this be manually tested?

A storybook example added

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?

#6640

Screenshots (if appropriate)

image

Do the grommet docs need to be updated?

No

Should this PR be mentioned in the release notes?

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

Compatible

@hwdsjtu97
Copy link
Contributor Author

@ShimiSun

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.

Nice work so far! It looks like there may be a few other places in Box.js where we may want to check if border is an array and it container side='between'. See line 103 of Box.js where it has if ( (border === 'between' || (border && border.side === 'between')) && !gap ). Also see line 221 where we have gap={ (boxOptions?.cssGap || cssGap) && gap && gap !== 'none' && border !== 'between' && border?.side !== 'between' && gap }

@hwdsjtu97
Copy link
Contributor Author

@jcfilben

@ShimiSun
Copy link
Collaborator

Nice work so far! It looks like there may be a few other places in Box.js where we may want to check if border is an array and it container side='between'. See line 103 of Box.js where it has if ( (border === 'between' || (border && border.side === 'between')) && !gap ). Also see line 221 where we have gap={ (boxOptions?.cssGap || cssGap) && gap && gap !== 'none' && border !== 'between' && border?.side !== 'between' && gap }

@jcfilben Do you think that the refinement requests you listed should block this current PR? or can we file a separate issue to track it?

@jcfilben
Copy link
Collaborator

@jcfilben Do you think that the refinement requests you listed should block this current PR? or can we file a separate issue to track it?

It looks like the changes requested already made their way into the PR. So no need to file a separate issue for this.

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!

@jcfilben jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PRty Biweekly PR reviews by grommet team with hoping of closing such PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants