-
Notifications
You must be signed in to change notification settings - Fork 36
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
Reduce bundle size #2
Conversation
style/main.scss
Outdated
ol { | ||
list-style: decimal; | ||
} | ||
|
||
ul { | ||
list-style: disc; | ||
} |
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.
This is added in f4c4470 and actually not necessary.
I assume you looked at a website with CSS reset (e.g. Tailwind's Preflight) and made the wrong assumption that they are needed.
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.
Why is this not needed? If you are using this on a site with Tailwind's preflight, won't the list style be unset?
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.
@primer/css
with github-markdown-css
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.
Looks great!
style/makefile
Outdated
patch: | ||
mkdir -p dist | ||
# Extract variables used by Markdown | ||
grep -Eoh -- "--[[:alnum:]-]+" node_modules/@primer/css/markdown/*.scss | sort -u > dist/variables | ||
# Extract variables used by ourselves | ||
grep -Eoh -- "--[[:alnum:]-]+" main.scss | sort -u >> dist/variables | ||
# Patch light colors | ||
echo "@mixin primer-colors-light {\n & {\n`grep -f dist/variables node_modules/@primer/primitives/dist/scss/colors/_light.scss`\n }\n}" > node_modules/@primer/primitives/dist/scss/colors/_light.scss | ||
# Patch dark colors | ||
echo "@mixin primer-colors-dark {\n & {\n`grep -f dist/variables node_modules/@primer/primitives/dist/scss/colors/_dark.scss`\n }\n}" > node_modules/@primer/primitives/dist/scss/colors/_dark.scss |
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.
Instead of this being a makefile, could you turn this into a Deno script?
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.
Done
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.
LGTM, thanks a lot!
Importing styles directly from
@primer/css
results in tons of unused CSS variables and rules being included in the bundle.This PR tries to reduce the bundle size by patching the colors stylesheet to only include colors we actually use: