Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

[React-UI] base style management #5732

Closed
seanforyou23 opened this issue Jun 13, 2019 · 8 comments
Closed

[React-UI] base style management #5732

seanforyou23 opened this issue Jun 13, 2019 · 8 comments
Labels
cat/bug A bug which needs fixing notif/triage The issue needs triage. Applied automatically to all new issues.

Comments

@seanforyou23
Copy link
Member

This is a...


[ ] Feature request
[x] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Documentation issue or request

The problem

Adding/removing a package that does the bulk of importing of third-party styling libs (syndesis/ui) in another sub-package changes the order in which styles are injected into the DOM for the app as a whole. So at the moment, the problem looks like the pf4 reset is coming after all the other styles. Seems like the app's base styles shouldn't be impacted by changes like this.

The problem is compounded by the fact that patternfly's react-core automatically injects every component stylesheet into the DOM regardless of it being used or not. Not a problem in production builds, but it's quite painful in dev mode when trying to debug styling issues.

I suppose I'm maybe making a case for introducing a null-loader to prevent packages from importing the pf4 react-core styles automatically just because they want to use one of the PF4 react components, and instead loading all the component/utility/layout styles directly from core. We could do this from the ui package only, and that way other sub packages could be dependent on it or not and in theory it wouldn't change anything about the styles

Expected behavior

Making a package dependent or no longer dependent on another package shouldn't affect the cascade of the base ui styles.

Screenshot

Screen Shot 2019-06-13 at 11 27 00 AM

@seanforyou23 seanforyou23 added the cat/bug A bug which needs fixing label Jun 13, 2019
@pure-bot pure-bot bot added the notif/triage The issue needs triage. Applied automatically to all new issues. label Jun 13, 2019
@gashcrumb
Copy link
Contributor

Just so it's clear so far this appears to be something that's happened to the app in dev mode, so far staging seems to look alright. But it's a bit troubling.

@riccardo-forina warning so you don't have a heart attack tomorrow morning or something

@riccardo-forina
Copy link
Collaborator

In prod mode everything gets simplified to plain old css files so I’m not that concerned. Do we know what introduced the regression? I’m quite sure it was all working as usual for me today. I might have seen something like the screenshot once but didn’t pay much attention to it because I was mid restart of the dev server and assumed that was the reason for the glitch

@gashcrumb
Copy link
Contributor

I've not quite figured it out, I also didn't see this until @seanforyou23 mentioned this was what master is like, since updating and rebuilding I see the same. Am just gonna peek at the latest commits now.

@gashcrumb
Copy link
Contributor

I rolled back my branch until it looked right, and when I apply this commit it breaks -> syndesisio/syndesis-react@516ae97

@riccardo-forina
Copy link
Collaborator

Hey mama look! I’m on tv!

Wonder, does it happen if you clean your application data with the dev tools?

@gashcrumb
Copy link
Contributor

🤣

@gashcrumb
Copy link
Contributor

PR incoming, no worries.

@seanforyou23
Copy link
Member Author

I created the following issue to take the onus off of syndesis team to do this work, it's really a generic problem of how consumers can pull in pf styles, so I think we can close this issue on our end.

patternfly/patternfly-react-seed#47

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cat/bug A bug which needs fixing notif/triage The issue needs triage. Applied automatically to all new issues.
Projects
None yet
Development

No branches or pull requests

3 participants