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

Command to auto-generate CSS files from pygments styles in settings #641

Closed

Conversation

Shreya-7
Copy link
Contributor

@Shreya-7 Shreya-7 commented Oct 25, 2022

What GitHub issue does this PR apply to?

Resolves #602

What changed and why?

  1. Added a list of pygments styles to be offered to the user. Chosen from the accessibility-friendly section of this list: https://pygments.org/styles/
  2. Added a management command to auto-generate CSS files from the above list, making adding and updating styles MUCH easier.
  3. Changed the current syntax highlighting to use the default style instead of colorful as it has better contrast (thus helping with accessibility.)

Checklist

  • I claimed any associated issue(s) and they are not someone else's
  • I have looked at documentation to ensure I made any revisions correctly
  • I tested my changes locally to ensure they work
  • (If editing Django) I have added or edited any appropriate unit tests for my changes

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

@Shreya-7 Shreya-7 changed the title COmmand to auto-generate CSS files from pygments styles in settings Command to auto-generate CSS files from pygments styles in settings Oct 25, 2022
@Shreya-7
Copy link
Contributor Author

@cafce25 can you review this PR?
Also, do we want to do the user style selection as part of a separate PR (since it's a different piece of functionality) or should I work on that as part of this one itself?

Copy link
Contributor

@cafce25 cafce25 left a 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.

web/management/commands/generate_css_files.py Outdated Show resolved Hide resolved
web/management/commands/generate_css_files.py Outdated Show resolved Hide resolved
@Shreya-7
Copy link
Contributor Author

Shreya-7 commented Oct 26, 2022

@cafce25

It might be worth investigating whether we should be versioning these auto generated css files

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?

or if dynamically generating the styles in a new view wouldn't make more sense.

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?

@Shreya-7 Shreya-7 requested a review from cafce25 October 26, 2022 17:24
@cafce25
Copy link
Contributor

cafce25 commented Oct 26, 2022

It might be worth investigating whether we should be versioning these auto generated css files

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?

Sorry I meant check in, not version.

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 mean have an endpoint similar to styles/pygments/<stylename> that serves the css. No need for templates in this case.

I'm not sure we would want to generate the CSS every time a new view is requested.

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.

@Shreya-7
Copy link
Contributor Author

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.

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?

I mean have an endpoint similar to styles/pygments/ that serves the CSS.

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 :)

@geekygirlsarah geekygirlsarah temporarily deployed to codethesaurus-test-pr-641 October 29, 2022 15:01 Inactive
@geekygirlsarah geekygirlsarah temporarily deployed to codethesaurus-test-pr-641 October 29, 2022 15:04 Inactive
@geekygirlsarah geekygirlsarah temporarily deployed to codethesaurus-test-pr-641 October 29, 2022 19:05 Inactive
@geekygirlsarah
Copy link
Member

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:

  1. Where are the CSS files you're adding to the repo coming from?
  2. What is happening to the CSS files you're autogenerating?
  3. I saw the change from colorful to default but not seeing about the other styles and when/where they're getting used.
  4. So is conversation shifting towards a dropdown that the user of the site picks the style they want and it's saved in a cookie? (And the functionality isn't in this PR yet?)

@cafce25
Copy link
Contributor

cafce25 commented Oct 29, 2022

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:

1. Where are the CSS files you're adding to the repo coming from?

I assume they were generated with the added command.

2. What is happening to the CSS files you're autogenerating?

At the moment they just get stored in the file system to be commited by a user.

3. I saw the change from `colorful` to `default` but not seeing about the other styles and when/where they're getting used.
4. So is conversation shifting towards a dropdown that the user of the site picks the style they want and it's saved in a cookie? (And the functionality isn't in this PR yet?)

Correct, both of these are not in this PR yet.

@geekygirlsarah geekygirlsarah marked this pull request as draft October 29, 2022 19:14
@geekygirlsarah
Copy link
Member

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.)

@cafce25
Copy link
Contributor

cafce25 commented Oct 29, 2022

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.

@geekygirlsarah
Copy link
Member

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.

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.

Improve Syntax highlighting
3 participants