-
Notifications
You must be signed in to change notification settings - Fork 4
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.
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
[release/10.11.0] PBI AB#97569 [Tech Debt] - Healthy Church Widgets Framework - Ensure CSS shipped with widgets CANNOT affect anything on consuming page, outside of widget's container #495
Conversation
src/style.scss
Outdated
@@ -1,3 +1,4 @@ | |||
.react-cm-ui { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe think a bit about this name.
We might want like .hc
or .healthy-church
or .hc-ui
or .healthy-church-ui
.
Thoughts?
cc
@JLeeC
@sircris
@War777
@KacosPro
@razmik-medoxi
@absqueued
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder why we wrapped modular styles with a class ?
@IlyaRadinsky can you please explain what are the benefits of doing it and why we couldn't do it in another way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@razmik-medoxi This is in service of Product Backlog Item 97569: [Tech Debt] - Healthy Church Widgets Framework - Ensure CSS shipped with widgets CANNOT affect anything on consuming page, outside of widget's container. So, for example, let's say React CM UI styles <h1>
elements and other really common elements. When we export CSS from React CM UI and bundle it as part of a widget intended for use on an external website (e.g. saddleback.com), that style will "spill over" from the widget and affect the consuming website, which is highly undesirable. So this is designed to prevent that.
See for example:
https://hc-example-website.azurewebsites.net/ (no widgets)
https://hc-example-website.azurewebsites.net/Form (has the Forms widget on it)
Compare for example the top navigation bar (text gets bolder and such).
This is due to widget CSS (which includes React CM UI CSS). That's what we're trying to prevent here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, I thought we encapsulated the widgets but it seems the styling didn't work well with that solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@razmik-medoxi This is like attempt number 3 to solve this problem! Prior attempts from @IlyaRadinsky and @KacosPro have not panned out. This is a tough dragon to slay apparently. But this seems promising, at least to me.
Also, please see my comment above; do you have thoughts on a good name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.hc-ui
is the number 1 candidate for me =))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, looks good to me. Want to stack hands on the class name we use.
I trust we don't expect any obvious ill effects on the HC Admin UI side?
I see you added this class ( https://github.com/saddlebackdev/church-management/pull/9643, specifically https://github.com/saddlebackdev/church-management/pull/9643/files#diff-e13bb9ab1347edb6e8fdafe64e5e05102c588e201f6c9da7c94c14caeb0bc2fbL2-R2).
Wondering if, whatever class we decide, we should declare a constant (like |
NOTE: We might want to start a |
…to features/97569-css-bundle
@groberts314 I tried, but didn't find a way to use |
@IlyaRadinsky It's fine if you have to have it separate for SCSS -- I was mostly thinking of places where it is used in JS/TS. |
* Starting a release candidate branch for React CM UI 10.11.0. * Will re-target #495 to this release candidate branch and publish a prerelease package from the feature branch.
… style.scss under .hc-ui class
@groberts314 could you please deploy my last updates? I found another place where |
Released latest changes in here as |
Deployed latest to HC-Feature. Still looks like it's not loading Source Sans Pro. |
@groberts314 deploy my last updates, please |
Latest React CM UI changes released as |
Deployed to HC-Feature.
|
Removing We verified:
|
[dev] PBI AB#97569 Try and fix UI Docs website after #495
Added a global CSS class to wrap all exported styles in a single scope