-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
test: ensure consistent CSS ordering #6222
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
ef15220
Test website CSS ordering
slorber bd2947b
Add some infima classes to the CSS order test
slorber 1684b07
fix error message
slorber b08783b
fix infima classname
slorber 3bbe21d
Add more CSS order test classes
slorber 74f3e64
Add more CSS order test classes
slorber 4f4f571
cleanup css order test
slorber File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/* Used to test CSS insertion order */ | ||
.test-marker-site-client-module { | ||
content: "site-client-module"; | ||
} |
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
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
import React from 'react'; | ||
import type {Props} from '@theme/Layout'; | ||
import Layout from '@theme-original/Layout'; | ||
|
||
// This component is only used to test for CSS insertion order | ||
import './styles.module.css'; | ||
|
||
export default function LayoutWrapper(props: Props): JSX.Element { | ||
return <Layout {...props} />; | ||
} |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
/* Used to test CSS insertion order */ | ||
.test-marker-theme-layout { | ||
content: "theme-layout"; | ||
} | ||
|
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,110 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
*/ | ||
|
||
const path = require('path'); | ||
const fs = require('fs'); | ||
|
||
/* | ||
This verifies CSS ordering on the Docusaurus site itself, | ||
There are multiple ways to provide some CSS to Docusaurus | ||
and Docusaurus should guarantee a consistent CSS ordering over time | ||
See also | ||
- https://github.com/facebook/docusaurus/issues/3678 | ||
- https://github.com/facebook/docusaurus/pull/5987 | ||
TODO we should probably add a real e2e test in core instead of using our own website? | ||
Current solution looks good-enough for now | ||
*/ | ||
|
||
// TODO temporary, the current order is bad and we should change/fix that | ||
const EXPECTED_CSS_MARKERS = [ | ||
// Note, Infima and site classes are optimized/deduplicated and put at the top | ||
// We don't agree yet on what should be the order for those classes | ||
// See https://github.com/facebook/docusaurus/pull/6222 | ||
'.markdown>h2', | ||
'.button--outline.button--active', | ||
'.DocSearch-Hit-content-wrapper', | ||
'.navbar__title', | ||
'--ifm-color-scheme:light', | ||
'.test-marker-site-custom-css-shared-rule', | ||
'.col[class*=col--]', | ||
'.padding-vert--xl', | ||
'.footer__link-item', | ||
'.pagination__item', | ||
'.pills__item', | ||
'.tabs__item', | ||
|
||
// Test markers | ||
'.test-marker-site-custom-css-unique-rule', | ||
'.test-marker-site-client-module', | ||
'.test-marker-theme-layout', | ||
'.test-marker-site-index-page', | ||
|
||
// lazy loaded lib | ||
'.DocSearch-Modal', | ||
]; | ||
|
||
const cssDirName = path.join(__dirname, 'build', 'assets', 'css'); | ||
|
||
const cssFileNames = fs | ||
.readdirSync(cssDirName) | ||
.filter((file) => file.endsWith('.css')); | ||
|
||
if (cssFileNames.length !== 1) { | ||
throw new Error('unexpected: more than 1 css file'); | ||
} | ||
const cssFile = path.join(cssDirName, cssFileNames[0]); | ||
|
||
console.log('Inspecting CSS file for test CSS markers', cssFile); | ||
|
||
const cssFileContent = fs.readFileSync(cssFile, 'utf8'); | ||
|
||
const cssMarkersWithPositions = EXPECTED_CSS_MARKERS.map((marker) => { | ||
const position = cssFileContent.indexOf(marker); | ||
return {marker, position}; | ||
}); | ||
|
||
const missingCSSMarkers = cssMarkersWithPositions | ||
.filter((m) => m.position === -1) | ||
.map((m) => m.marker); | ||
|
||
if (missingCSSMarkers.length > 0) { | ||
throw new Error( | ||
`Some expected CSS marker classes could not be found in file ${cssFile}: \n- ${missingCSSMarkers.join( | ||
'\n- ', | ||
)}`, | ||
); | ||
} | ||
|
||
// https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore#_sortby-and-_orderby | ||
const sortBy = (key) => (a, b) => | ||
// eslint-disable-next-line no-nested-ternary | ||
a[key] > b[key] ? 1 : b[key] > a[key] ? -1 : 0; | ||
|
||
const sortedCSSMarkers = cssMarkersWithPositions | ||
.concat() | ||
.sort(sortBy('position')) | ||
.map(({marker}) => marker); | ||
|
||
if (JSON.stringify(sortedCSSMarkers) === JSON.stringify(EXPECTED_CSS_MARKERS)) { | ||
console.log(`Test CSS markers were found in the expected order: | ||
- ${sortedCSSMarkers.join('\n- ')}`); | ||
} else { | ||
throw new Error(`Test CSS markers were found in an incorrect order. | ||
Expected order: | ||
- ${EXPECTED_CSS_MARKERS.join('\n- ')}; | ||
Actual order: | ||
- ${sortedCSSMarkers.join('\n- ')}; | ||
CSS file: ${cssFile} | ||
`); | ||
} |
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.
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.
@lex111 @Josh-Cena as we can see the output CSS is currently in a weird order.
Due to CSS optimizations, Infima classes are re-ordered. Site custom CSS classes are "merged" with Infima classes sharing similar rules and put at the very beginning. We end up with CSS classes from different places (site, client modules, Infima, theme...) being interleaved in an unpredictable way.
I'm not sure if this is the correct thing to do in all cases, and I'd rather disable such optimizations, as it's better to do something that works than save a few Kb.
Are these optimizations always guaranteed 100% safe?
The main problem remains that site custom CSS should rather be last so that a site owner can more easily override existing CSS without
!important
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.
Ah, if it's because of optimization it makes perfect sense, and that's the entire purpose of optimization: combine duplicated classes where it can. If you have two class names that are identical in every way, it will be safe to combine them. I wouldn't worry about that since there's nothing to override in this case either.
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.
We already have a bloated bundle size that I've been looking for ways to minimize (e.g. #3655 could be a way out). Optimizations are meant to be optimizations: there aren't behavior differences but you can't expect everything to be the same, in the same way you can't rely on
someFunction.name
since it can be minified