-
Notifications
You must be signed in to change notification settings - Fork 168
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
Command to auto-generate CSS files from pygments styles in settings #641
Command to auto-generate CSS files from pygments styles in settings #641
Conversation
@cafce25 can you review this PR? |
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.
It might be worth investigating whether we should be versioning these auto generated css files or if dynamically generating the styles in a new view wouldn't make more sense.
If we do want the files in our repo then this looks good.
Multiple smaller PRs will be easier to merge and this does something on its own so it makes sense to do different PRs.
Hmm, I'm not sure if we would want to version the CSS files. I can't imagine anything would change in the stylesheets unless we're bumping our pygments version and then running this command?
Do you mean, generating the stylesheet within the view and passing it as a variable to the Jinja template? The CSS is static and if it remains unchanged for the majority of the time, I'm not sure we would want to generate the CSS every time a new view is requested. Plus, we'll be sending all the stylesheets from the view and manipulate the "active" one with JS, so that every time the user changes their style, we do not have to make a round-trip to the backend to fetch the new styles. Does that make sense? |
Sorry I meant check in, not version.
I mean have an endpoint similar to
That's what caching is for. It's not enabled yet but there is a pending pr for it. We also might get into trouble when pygments updates and our pre generated styles don't fit to the html it produces which might not be obvious and thus hard to catch. Before you act I think we should wait what @geekygirlsarah thinks about it. |
One thing we could do is add this new management command to our GitHub workflow so that the auto-generated CSS gets updated, making sure we're in sync whenever we decide to bump the pygments version we use. That might fix this problem?
Just to make sure I understand, we're saying that whenever the user selects a style from the dropdown, we would make a call to this new endpoint to fetch the CSS and store that style name in our cookie? And the next time the user visits the site, we would use this cookie value to fetch the CSS from the same endpoint again? For the rest, yeah let's wait for @geekygirlsarah to come back to us :) |
I guess I'm a bit far gone from the conversation that started this, as I'm finding I have a few questions and I'm not sure how to find the answers:
|
I assume they were generated with the added command.
At the moment they just get stored in the file system to be commited by a user.
Correct, both of these are not in this PR yet. |
Thanks @cafce25. So for starters, I'm marking this as a draft PR so it's easier for me to see it's not quite "done". Also are these styles not already in Pygments? Why are we making separate CSS files? (This might be me needing to read up on Pygments docs to fully understand but don't have the time to do it right now.) |
Yea that's what I was thinking too, we should generate the styles in a dynamic view instead of commiting them all to the repo. |
Since we haven't heard back from @Shreya-7 in over a month and this is a bit of a awkward issue/PR that's not really well defined and of the questions that have been brought up, not well answered either, I'm going to go ahead and close this PR. I'll leave the issue open a little longer to talk out things, but may close that back if we don't hear back from Shreya too. |
What GitHub issue does this PR apply to?
Resolves #602
What changed and why?
default
style instead ofcolorful
as it has better contrast (thus helping with accessibility.)Checklist
Any additional comments or things to be aware of while reviewing?
In terms of Lexers, I'm not sure how much better we can do after the ones provided by pygments