Skip to content

Conversation

ericsoderberghp
Copy link
Contributor

What does this PR do?

Addresses some issues with using grommet inside a shadow root context. Specifically, too much reliance on document being the root of things.

Also, added a "document" vs. "shadow" toolbar control in storybook to allow testing any story from a shadow root context.

Where should the reviewer start?

DropContainer.js

What testing has been done on this PR?

Various stories with the new shadow root context, certainly not all though.

How should this be manually tested?

stories with shadow root context

Do Jest tests follow these best practices?

no test changes

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

no

Should this PR be mentioned in the release notes?

yes

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

backwards compatible

@taysea
Copy link
Collaborator

taysea commented Mar 21, 2023

I like how in the current storybook, the theme selector says "Theme" -- I was expecting something similar in the deploy that said "Root". I noticed it's changed to say "grommet" and "document" which I didn't immediately recognize as a selector. soft opinion.

Screen Shot 2023-03-21 at 10 05 50 AM

NOW
Screen Shot 2023-03-21 at 10 09 29 AM

@ericsoderberghp ericsoderberghp self-assigned this Mar 22, 2023
@ericsoderberghp
Copy link
Contributor Author

I like how in the current storybook, the theme selector says "Theme" -- I was expecting something similar in the deploy that said "Root". I noticed it's changed to say "grommet" and "document" which I didn't immediately recognize as a selector. soft opinion.

Screen Shot 2023-03-21 at 10 05 50 AM

NOW Screen Shot 2023-03-21 at 10 09 29 AM

changed as suggested, I was soft too :)

@ericsoderberghp ericsoderberghp requested a review from taysea March 22, 2023 22:20
Copy link
Collaborator

@taysea taysea left a comment

Choose a reason for hiding this comment

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

LGTM.

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.

Sorry for the delay. Looks great.

@ericsoderberghp ericsoderberghp merged commit e054cbf into master Mar 23, 2023
@ericsoderberghp ericsoderberghp deleted the fix/shadow-root branch March 23, 2023 20:31
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.

3 participants