Skip to content
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

Provide complete CSS by leveraging fixture classes #39

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

earlAchromatic
Copy link
Contributor

Fixes sindresorhus/github-markdown-css#100 and lets us properly support permalinks, useful when using the GitHub Markdown API.

Also make sure we only scan declarations when checking for prettylights vars, otherwise it can crash on comment nodes.

This lets us properly support [rendering GitHub permalinks](sindresorhus/github-markdown-css#100), useful
when using the GitHub markdown API.

Also make sure we only scan declarations when checking for prettylights vars, otherwise it can crash on comment nodes.
@hyrious
Copy link
Collaborator

hyrious commented Nov 14, 2024

TIL the mode: 'gfm' param in GitHub API. It seems adds some GitHub stuff which are less seen outside (full diff):

+.markdown-body .Box-body.scrollable-overlay {
+  max-height: 400px;
+  overflow-y: scroll;
+}
+.markdown-body .commit-tease-sha {
+  display: inline-block;
+  font-family: ui-monospace, SFMono-Regular, SF Mono, Menlo, Consolas, Liberation Mono, monospace;
+  font-size: 90%;
+  color: #f0f6fc;
+}
+.markdown-body .blob-wrapper {
+  overflow-x: auto;
+  overflow-y: hidden;
+}

The Box component is used to display the code snippet (test).

I'm not sure whether adding them is right idea since people not using the API would less possible to generate HTML like that. On the other hand, the fixture could be outdated and not covering all possible cases that GitHub supports.

@earlAchromatic
Copy link
Contributor Author

I'm not sure whether adding them is right idea since people not using the API would less possible to generate HTML like that

Should we put it behind a useFixture option instead so that people can opt out?

On the other hand, the fixture could be outdated and not covering all possible cases that GitHub supports.

I think using the fixture would be less error prone than relying solely on a managed list of allowed classes. That way if the fixture reflects most common use cases, the output CSS is guaranteed to cover them even as GitHub updates their styles.

@hyrious
Copy link
Collaborator

hyrious commented Nov 14, 2024

Should we put it behind a useFixture option instead so that people can opt out?

LGTM 👍

Another idea is that these components are actually implemented in the primer package (GitHub open-sourced their front-end). They must starts-with an uppercase letter. Because usually our markdown renderer (either markdown-it, marked.js, remark, etc.) doesn't generate this kind of classes, they can be erased with a regex: /(\.|\[class^=)['"]?[A-Z]/, under a flag like components: false.

I think using the fixture would be less error prone than relying solely on a managed list of allowed classes.

You're right. We can improve that file in the future.

@hyrious hyrious requested a review from sindresorhus November 15, 2024 01:42
@sindresorhus
Copy link
Owner

It's not clear to be whether https://github.com/sindresorhus/github-markdown-css should use --no-use-fixture or not?

@hyrious
Copy link
Collaborator

hyrious commented Nov 15, 2024

@sindresorhus If you are using github-markdown.css with useFixture enabled, some part of the CSS only works if you're using GitHub's API to render your markdown file because the .Box component in GitHub's styling system doesn't seem commonly used outside of GitHub, which makes me think these parts aren't that necessary in the main CSS bundle.

Also it adds about 200 lines to the dark theme file which was 1100 lines.

We do have some CSS like this before. For example the syntax highlighting, which for now there is starry-night.

@earlAchromatic
Copy link
Contributor Author

I suppose it's a question of would you rather have github-markdown-css be more complete, or more lightweight?

I ended up with unstyled markdown elements in production because it used to be more complete, but when I updated to newer version it was more stripped down and I didn't catch it. I think I would default to complete and let people add the generator to their pipeline if they need to save the extra KB.

We could instead generate an extra file in github-markdown-css that supplies the styles for when you are using the GitHub Markdown API. That would require some changes so the useFixture option works like onlyStyles/onlyVariables.

@sindresorhus sindresorhus merged commit 73bb29f into sindresorhus:main Nov 15, 2024
2 checks passed
@LolipopJ
Copy link

This new feature may lead to an unexpected behavior on .markdown-body:

sindresorhus/github-markdown-css@0b0d6ed#diff-58c14b684074ea65e9c09eab2b3b97a65c5f940e8cffa533bd032e988b59d02bR135

Which add an additional style property min-height: 100vh (sindresorhus/github-markdown-css#115)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

permalinks
4 participants