Skip to content

fix: issue with custom styles piercing shadow DOM rendering #1365

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.

Sign up for GitHub

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

Jump to bottom
Merged
castastrophe merged 29 commits into from
Feb 18, 2021
Merged

fix: issue with custom styles piercing shadow DOM rendering #1365

castastrophe merged 29 commits into from
Feb 18, 2021

Conversation

@castastrophe
Copy link
Contributor

@castastrophe castastrophe commented Feb 16, 2021 •
edited
Loading

tldr;

Content set had moved the tabs and accordion into the shadow DOM for controlled rendering but found that this strips all styles from the panels. Not good!! This patch reverts that change and keeps the tabs and accordion in the light DOM so that panel styles will continue to work.

Preview

Testing instructions

  1. Regression testing on pfe-content-set
  2. Add custom styles to the panel region and ensure those styles persist when the content set upgrades.

Browser requirements

Your component should work in all of the following environments:

  • Latest 2 versions of Edge
  • Internet Explorer 11 (should be useable, not pixel perfect)
  • Latest 2 versions of Firefox (one on Mac OS, one of Windows OS)
  • Firefox 68 (or latest version for Red Hat Enterprise Linux distribution)
  • Latest 2 versions of Chrome (one on Mac OS, one of Windows OS)
  • Latest 2 versions of Safari
  • Android mobile device (such as the Galaxy S9)
  • Apple mobile device (such as the iPhone X)
  • Apple tablet device (such as the iPhone Pro)

Ready-for-merge Checklist

  • Expected files: all files in this pull request are related to one request or issue (no stragglers or scope-creep).
  • Tests have been updated to cover these changes.
  • Browser testing passed.
  • Repository compiles and tests pass.
  • Changelog updated.

Merging

Please squash when merging and ensure your commit message uses conventional commit formatting.

Be sure to share your updates with the patternfly-elements-contribute@redhat.com mailing list!

@github-actions github-actions bot added the functionality Functionality, typically pertaining to the JavaScript. label Feb 16, 2021
@castastrophe castastrophe added bug next release PRs that need to merge before the next release goes out priority: high Severity level: 1 needs changelog Be sure to update the Changelog before merging. ready: branch testing Test the component from a user-perspective. Try to break it! ready: browser testing Test the component in the supported browser environments. ready: code review Ready for code review! labels Feb 16, 2021
@castastrophe castastrophe added this to the 1.x milestone Feb 16, 2021
@castastrophe castastrophe self-assigned this Feb 16, 2021
@heyMP heyMP self-requested a review February 16, 2021 21:22
heyMP
heyMP reviewed Feb 16, 2021
View reviewed changes
Copy link
Contributor

@heyMP heyMP left a comment

Choose a reason for hiding this comment

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

Really creative use of the slots! 🤹

@github-actions github-actions bot added demo Updating demo pages styles An issue or PR pertaining only to CSS/Sass labels Feb 16, 2021
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
@github-actions github-actions bot added the tests Related to testing label Feb 17, 2021
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
castastrophe
castastrophe commented Feb 17, 2021
View reviewed changes
@castastrophe castastrophe removed needs changelog Be sure to update the Changelog before merging. ready: browser testing Test the component in the supported browser environments. labels Feb 17, 2021
@starryeyez024 starryeyez024 linked an issue Feb 17, 2021 that may be closed by this pull request
bug with pfe-content-set moving panel content into the shadow DOM #1371
Closed
@castastrophe castastrophe mentioned this pull request Feb 17, 2021
Closed
@castastrophe castastrophe added the run e2e Trigger automated visual regression tests label Feb 17, 2021
heyMP
heyMP approved these changes Feb 17, 2021
View reviewed changes
@castastrophe castastrophe removed the ready: code review Ready for code review! label Feb 17, 2021
@castastrophe castastrophe enabled auto-merge (squash) February 17, 2021 21:19
starryeyez024
starryeyez024 approved these changes Feb 17, 2021
View reviewed changes
Copy link
Member

@starryeyez024 starryeyez024 left a comment

Choose a reason for hiding this comment

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

Life: Gift To Mankind

@castastrophe
Copy link
Contributor Author

castastrophe commented Feb 18, 2021

1 more test to fix: https://github.com/patternfly/patternfly-elements/runs/1922191577#step:8:2022

@castastrophe castastrophe merged commit 51bee47 into master Feb 18, 2021
@castastrophe castastrophe deleted the fix-content-set-shadow-styles branch February 18, 2021 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@starryeyez024 starryeyez024 starryeyez024 approved these changes

@heyMP heyMP heyMP approved these changes

Assignees

@castastrophe castastrophe

Labels

demo Updating demo pages functionality Functionality, typically pertaining to the JavaScript. next release PRs that need to merge before the next release goes out priority: high Severity level: 1 ready: branch testing Test the component from a user-perspective. Try to break it! ready to merge run e2e Trigger automated visual regression tests styles An issue or PR pertaining only to CSS/Sass tests Related to testing

Projects

None yet

Milestone

1.x

Development

Successfully merging this pull request may close these issues.

bug with pfe-content-set moving panel content into the shadow DOM

4 participants

@castastrophe @starryeyez024 @heyMP