Skip to content
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

Merged
merged 10 commits into from
May 23, 2024

Conversation

IlyaRadinsky
Copy link
Contributor

Added a global CSS class to wrap all exported styles in a single scope

src/style.scss Outdated
@@ -1,3 +1,4 @@
.react-cm-ui {
Copy link
Contributor

@groberts314 groberts314 Apr 26, 2024

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

Copy link
Contributor

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 ?

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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?

Copy link
Contributor

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 =))

Copy link
Contributor

@groberts314 groberts314 left a 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).

@groberts314
Copy link
Contributor

Wondering if, whatever class we decide, we should declare a constant (like UI_LIBRARY_ROOT_CLASS or something) and export it.
Thoughts?

@groberts314
Copy link
Contributor

NOTE: We might want to start a release/10.10.0 RC branch, re-target this PR into that branch, and then publish a pre-release/beta version of React CM UI NPM package to test with. Thoughts?

@groberts314 groberts314 added priority: medium Regular priority for code review risk: medium Medium Risk/Criticality labels Apr 26, 2024
@IlyaRadinsky
Copy link
Contributor Author

Wondering if, whatever class we decide, we should declare a constant (like UI_LIBRARY_ROOT_CLASS or something) and export it. Thoughts?

@groberts314 I tried, but didn't find a way to use UI_LIBRARY_ROOT_CLASS constant from the JS in SCSS

@groberts314
Copy link
Contributor

@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.

groberts314 added a commit that referenced this pull request Apr 30, 2024
* 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.
@groberts314 groberts314 changed the title [dev] 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 [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 Apr 30, 2024
@groberts314 groberts314 changed the base branch from dev to release/10.11.0 April 30, 2024 22:42
@IlyaRadinsky
Copy link
Contributor Author

@groberts314 could you please deploy my last updates? I found another place where react-cm-ui affects to the body and html elements, and this should fix the font in about page

@groberts314
Copy link
Contributor

Released latest changes in here as 10.11.0-rc3.

@groberts314
Copy link
Contributor

Deployed latest to HC-Feature. Still looks like it's not loading Source Sans Pro.

@IlyaRadinsky
Copy link
Contributor Author

@groberts314 deploy my last updates, please

@groberts314
Copy link
Contributor

Latest React CM UI changes released as 10.11.0-rc4

@groberts314
Copy link
Contributor

Deployed to HC-Feature.
Looks like that did the trick for the custom font.
Now we gotta test:

  1. Does HC Admin look good? No obvious regressions?
  2. Is a website using the widgets looking good? (i.e. did we fix the issue?)

@groberts314
Copy link
Contributor

Removing do not merge
Jessica Titov and I verified HC-Feature with these changes and https://github.com/saddlebackdev/church-management/pull/9643 deployed ( UI build 1.89.0-FT-91e61c6.20240515.3 ).

We verified:

  • No major regressions in HC Admin UI
  • This finally fixed the widget CSS spillover issue (verified that the previous notable changes in Example Website's top header no longer occur)

@groberts314 groberts314 merged commit b5aba06 into release/10.11.0 May 23, 2024
2 of 3 checks passed
@groberts314 groberts314 deleted the features/97569-css-bundle branch May 23, 2024 20:44
groberts314 added a commit that referenced this pull request May 24, 2024
groberts314 added a commit that referenced this pull request Aug 2, 2024
[dev] PBI AB#97569 Try and fix UI Docs website after #495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement priority: medium Regular priority for code review risk: medium Medium Risk/Criticality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants