fix(flamegraph): fix its styling #1388
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1388 +/- ##
==========================================
+ Coverage 67.60% 67.66% +0.06%
==========================================
Files 123 123
Lines 4040 4047 +7
Branches 930 932 +2
==========================================
+ Hits 2731 2738 +7
Misses 1305 1305
Partials 4 4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
size-limit report 📦
|
|
/create-server |
|
/create-server |
eh-am
force-pushed
the
chore/fix-flamegraph-css
branch
from
56009a9
to
d34f3b4
Compare
|
/create-server |
|
/create-server |
petethepig
reviewed
Aug 12, 2022
| @@ -0,0 +1,38 @@ | |||
| name: Storybook | |||
petethepig
approved these changes
Aug 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@pyroscope/flamegraphandwebapphave the same styling rules (mostly the normalization ones)sanitize.css, for some reason I could make it so that a nestedimport ~sanitize.csswould be inlined (tried with postcss plugin and nothing)The original problem is that the webapp and flamegraph (standalone) shipped different sets of CSS. Which means it would look just fine in webapp (which is what most people would be running), then in the standalone version (imported via
@pyroscope/flamegraphpackage), it would look slightly off.Turns out the difference is that the flamegraph was not shipping the styles from normalize.css.
However, we can't just ship those styles, since they are global and they may affect end user's style (imagine I embed the flamegraph into my component, and then somehow it overrides my app's button!).
Therefore we need to scope those rules.
However, if we scope those rules, then they start to override the ones from css modules. The reason is simple.
Consider a sanitize.css rule for
table:If we scope under
.pyroscopeit will become:Then let's say we have a css modules rule, which normally would override the one from sanitize css (since class has higher precedence over a simple tag)
In our case,
.pyroscope tableis higher than.oiJX5Zp1+W8aJOcWSU-baQ==, therefore our css module style would never be applied.To "solve" this I ended up using a "custom" element (not confuse with real custom elements), here called
pyro-flamegraph.That way,
pyro-flamegraph tablestill has a lower specificity than.oiJX5Zp1+W8aJOcWSU-baQ==, restoring the original behavior of having classes generated by CSS modules to overwrite the styles from the reset.Disclaimer
This may break for people that assume the flamegraph component has a certain shape, eg in snapshot tests.
Since we publish the component as < 1.0.0, I won't publish as a breaking change (even though it is).