[WIP] Make media query and :visited CSS rules unique #635

tptee · 2016-03-21T21:17:57Z

Resolves #593 and #625.

@ianobermiller @alexlande can you give this a sanity check?

coveralls · 2016-03-21T21:24:35Z

Coverage decreased (-0.09%) to 95.139% when pulling 6bf2895 on fix-duplicate-css into 82be93a on master.

ianobermiller · 2016-03-21T21:29:28Z

src/plugins/resolve-media-queries-plugin.js

@@ -73,7 +74,7 @@ function _topLevelRulesToCSS({
    );

    // CSS classes cannot start with a number
-    const mediaQueryClassName = 'rmq-' + hash(query + ruleCSS);
+    const mediaQueryClassName = `rmq-${styleId()}-${hash(query + ruleCSS)}`;


styleId() will execute every render. Since addCSS no longer dedupes due to the incrementing number, you'll end up with many duplicate styles in the CSS, right?

Sure looks that way. The unique component id route may solve that issue, and I think it'd be a similar sized diff. Thoughts?

This reverts commit 6bf2895.

…ponent props once, allowing for deduping

tptee · 2016-03-21T22:40:23Z

The approach I just pushed no longer uses a counter, but a uuid set only once on component props. This gives us deduping back and prevents coupling the plugins to the StyleKeeper. Thoughts?

coveralls · 2016-03-21T22:50:51Z

Coverage increased (+0.04%) to 95.271% when pulling 9932a59 on fix-duplicate-css into 82be93a on master.

ianobermiller · 2016-03-21T23:23:38Z

You could also store the unique id in the component state (directly on the
instance).
On Mon, Mar 21, 2016 at 3:50 PM Coveralls [email protected] wrote:

[image: Coverage Status] https://coveralls.io/builds/5496943

Coverage increased (+0.04%) to 95.271% when pulling 9932a59
9932a59
on fix-duplicate-css into 82be93a
82be93a
on master.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#635 (comment)

…instances (won't run every render, which also means we don't have to check if it already exists!)

tptee · 2016-03-22T05:07:47Z

That's so much better, now it doesn't run every render and we don't have to check that we already set the styleID on the component. Thanks for the guidance!

coveralls · 2016-03-22T05:13:01Z

Coverage increased (+0.03%) to 95.255% when pulling db89d04 on fix-duplicate-css into 82be93a on master.

… EPEERINVALID

coveralls · 2016-03-22T05:34:57Z

Coverage increased (+0.03%) to 95.255% when pulling 65444c7 on fix-duplicate-css into 82be93a on master.

ianobermiller · 2016-03-22T17:19:37Z

src/enhancer.js

@@ -63,6 +64,7 @@ export default function enhanceWithRadium(

      this.state = this.state || {};
      this.state._radiumStyleState = {};
+      this.state._styleID = uuid.v4();


Do we want this to be per-instance or per-component? #625 would be solved by per-component, but #593 it seems would not be.

If it is per-instance, you should move it directly to this, and prefix it with radium, just like _radiumIsMounted below.

If it is per component class, you can make it a static, like _isRadiumEnhanced above.

Ah right, confirmed a static _styleID doesn't solve #625

ianobermiller · 2016-03-22T17:21:38Z

One issue with putting a styleID on each instance is that you'd end up with a ton of bloat and duplication in the stylesheet; consider rendering a grid with 100 cells, each one adding 2-3 media queries.

tptee · 2016-03-23T04:33:58Z

True, that would be nasty. Maybe we can change the StyleKeeper's _cssSet to look like { cssClass: rule }. getCSS can then detect which classes have the same rule and generate CSS to reuse the rule like so:

@media only screen and (min-width: 1024) {
  .rmq-12345, .rmq-23456 {
    display: none;
  }
}

I'll investigate!

tptee · 2016-03-23T04:39:37Z

Does _styleID as a static on the component class avoid the stylesheet bloat? I'm fuzzy on why it would/wouldn't

ianobermiller · 2016-03-26T05:33:52Z

It would avoid stylesheet bloat, but might not fix usage with grid for example, where you had clashes within the same component, right?

tptee · 2016-03-27T06:20:03Z

I tried a few approaches to reducing styleID's added bloat to the stylesheet without success. Using styleID increases the lines of CSS from 315 to 896 (~3x) in the radium-grid demo. However, since CSS parsing is quick (to my knowledge), this might be a worthwhile tradeoff for correctness's sake until we devise a better solution.

It may be worth reconsidering media query precedence in the future, as I've noticed designers heavily rely on the cascading behavior of min-width media query ranges. We could more aggressively consolidate media query rules while enforcing the usual definition-order precedence for rules within the media query.

I honestly don't know which follows the principle of least surprise: the consistency of definition-order precedence everywhere, or the cascading behavior of media query ranges that designers expect.

Thoughts? @ianobermiller @alexlande

ianobermiller · 2016-03-28T20:26:07Z

@tptee, the 3x will be fine after gzip, but having too many selectors will definitely slow things down eventually. We need a better solution, but I am fine with this until we figure one out. The increase is linear with respect to what? The number of components rendered? If you have a grid with 100 rows, do you get 100x the style sheet size?

tptee · 2016-06-06T04:21:34Z

I'm going to kick the tires on a more robust solution, since I don't have a clear picture of the scale at which stylesheet bloat increases. I'll hopefully have something to report back soon!

tptee · 2016-06-12T06:24:28Z

I think I have an optimization for removing stylesheet bloat that still guarantees rule uniqueness. I've got a brain dump below, and I'd love if you all could give this a feasibility check before I continue. It's the only optimization I could find that respects the order precedence test.

/cc @ianobermiller @alexlande @coopy

/*

Algorithm:

- Prepare an incoming media query + ruleset using addCSS().
- Look at the previous rules defined for the same media query (if present).
- Remove rules in the current set that are duplicates of the previous set.
- Write the deduplicated current set to the stylesheet.
- Apply the class names of both the previous and current set to the component.

*/

/* Existing query */
@media only screen and (max-width: 640px) {
    .rmq-ffe912bc {
        display: flex !important;
        flex-basis: calc(50% - 8px) !important;
        align-items: flex-start !important;
        justify-content: flex-start !important;
        order: initial !important;
    }
}

/* Incoming query */
@media only screen and (max-width: 640px) {
    .rmq-69620a79 {
        display: flex !important;
        flex-basis: 100% !important;
        align-items: flex-start !important;
        justify-content: flex-start !important;
        order: initial !important;
    }
}

/* duplicates: display, align-items, justify-content, order */

/* component <---- .rmq-ffe912bc .rmq-69620a79 */
/* deduped ruleset ------> stylesheet */
@media only screen and (max-width: 640px) {
    .rmq-69620a79 {
        flex-basis: 100% !important;
    }
}

/* --------------------------------------------------------- */

/* Another incoming query */
@media only screen and (max-width: 640px) {
    .rmq-b0a85267 {
        display: flex !important;
        flex-basis: 100% !important;
        align-items: center !important;
        justify-content: flex-end !important;
        order: initial !important;
    }
}

/* duplicates from first query: display */
/* duplicates from second query: flex-basis */

/* component <---- .rmq-ffe912bc .rmq-69620a79 .rmq-b0a85267 */
/* deduped ruleset ------> stylesheet */
@media only screen and (max-width: 640px) {
    .rmq-b0a85267 {
        align-items: center !important;
        justify-content: flex-end !important;
    }
}

ianobermiller · 2016-06-13T03:16:16Z

Sounds complicated, but I think it would work.

alexlande · 2016-06-13T17:37:57Z

Yep, sounds reasonable to me.

twoam · 2016-09-30T09:42:06Z

Any recent update regarding this PR? Cheers.

kylecesmat · 2019-09-05T17:52:40Z

Closing this as it will not be prioritized and the branch has fallen stale

Make media query and :visited CSS rules unique

6bf2895

ianobermiller reviewed Mar 21, 2016
View reviewed changes

tptee added 2 commits March 21, 2016 15:24

Revert "Make media query and :visited CSS rules unique"

0e2f588

This reverts commit 6bf2895.

Use styleID plugin with uuids instead of counter; IDs only set on com…

9932a59

…ponent props once, allowing for deduping

Move styleID from a plugin to a state variable on enhanced component …

db89d04

…instances (won't run every render, which also means we don't have to check if it already exists!)

Add lolex as a devDependency to prevent failed appveyor builds due to…

65444c7

… EPEERINVALID

ianobermiller reviewed Mar 22, 2016
View reviewed changes

alexlande mentioned this pull request Mar 29, 2016

Overlapping media queries order can cause issues due to the use of non-unique class hashes #625

Open

tptee mentioned this pull request Jun 6, 2016

Larger screen inherit settings from smaller screen settings? FormidableLabs/radium-grid#48

Open

alexlande added the high priority label Jun 13, 2016

tptee self-assigned this Jun 14, 2016

tptee added the in progress label Jun 14, 2016

jeffrifwald mentioned this pull request Jun 30, 2016

Media query breakpoint does not work #756

Closed

This was referenced Jul 19, 2016

The Road to 1.0 #788

Open

Generated CSS Ordering #790

Open

ryan-roemer changed the title ~~Make media query and :visited CSS rules unique~~ [WIP] Make media query and :visited CSS rules unique Sep 14, 2017

alexlande mentioned this pull request Sep 16, 2017

zIndex doesn't work #851

Closed

rawrmonstar mentioned this pull request Dec 20, 2018

Fix media query ordering #1015

Closed

kylecesmat closed this Sep 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Make media query and :visited CSS rules unique #635

[WIP] Make media query and :visited CSS rules unique #635

tptee commented Mar 21, 2016

coveralls commented Mar 21, 2016

ianobermiller Mar 21, 2016

tptee Mar 21, 2016

tptee commented Mar 21, 2016

coveralls commented Mar 21, 2016

ianobermiller commented Mar 21, 2016

tptee commented Mar 22, 2016

coveralls commented Mar 22, 2016

coveralls commented Mar 22, 2016

ianobermiller Mar 22, 2016

tptee Mar 23, 2016

ianobermiller commented Mar 22, 2016

tptee commented Mar 23, 2016

tptee commented Mar 23, 2016

ianobermiller commented Mar 26, 2016

tptee commented Mar 27, 2016

ianobermiller commented Mar 28, 2016

tptee commented Jun 6, 2016

tptee commented Jun 12, 2016

ianobermiller commented Jun 13, 2016

alexlande commented Jun 13, 2016

twoam commented Sep 30, 2016

kylecesmat commented Sep 5, 2019

[WIP] Make media query and :visited CSS rules unique #635

[WIP] Make media query and :visited CSS rules unique #635

Conversation

tptee commented Mar 21, 2016

coveralls commented Mar 21, 2016

ianobermiller Mar 21, 2016

Choose a reason for hiding this comment

tptee Mar 21, 2016

Choose a reason for hiding this comment

tptee commented Mar 21, 2016

coveralls commented Mar 21, 2016

ianobermiller commented Mar 21, 2016

tptee commented Mar 22, 2016

coveralls commented Mar 22, 2016

coveralls commented Mar 22, 2016

ianobermiller Mar 22, 2016

Choose a reason for hiding this comment

tptee Mar 23, 2016

Choose a reason for hiding this comment

ianobermiller commented Mar 22, 2016

tptee commented Mar 23, 2016

tptee commented Mar 23, 2016

ianobermiller commented Mar 26, 2016

tptee commented Mar 27, 2016

ianobermiller commented Mar 28, 2016

tptee commented Jun 6, 2016

tptee commented Jun 12, 2016

ianobermiller commented Jun 13, 2016

alexlande commented Jun 13, 2016

twoam commented Sep 30, 2016

kylecesmat commented Sep 5, 2019