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

Improve Syntax highlighting #602

Open
cafce25 opened this issue Oct 18, 2022 · 19 comments
Open

Improve Syntax highlighting #602

cafce25 opened this issue Oct 18, 2022 · 19 comments
Labels
Experience: Design good first issue A good issue for anyone new to open source to work on help wanted We'd love to have help working on this issue Type: Enhancement

Comments

@cafce25
Copy link
Contributor

cafce25 commented Oct 18, 2022

Expected Behavior

Syntax highlighting does a good job and highlights strings, variables, function/method calls,

Actual Behavior

Only a few keywords and strings are highlighted, the strings barely noticable.
bad highlight example

Steps to Reproduce the Problem

  1. Visit any reference / comparision page

Additional Info

There is a few things we can look into here:

  1. Change the pygments color scheme we use so the few things that are highligted are clearly visible
  2. Look into pygments settings if we can get more things highligted (different Lexer?)
  3. Contribute to upstream pygments to get better highligting
  4. Switch from pygments to something else entirely
@geekygirlsarah
Copy link
Member

I think like you mentioned, some of it is we have code snippets and not full pieces of running code, so their highlighters might not fully know everything to highlight.

3/4 are outside of the scope of a ticket here. We can see if anyone wants to look into 1/2.

@geekygirlsarah geekygirlsarah added Type: Enhancement help wanted We'd love to have help working on this issue hacktoberfest Good issue for someone to work on for Hacktoberfest Experience: Design good first issue A good issue for anyone new to open source to work on labels Oct 18, 2022
@Shreya-7
Copy link
Contributor

@geekygirlsarah I wouldn't mind looking into 1/2!

@cafce25
Copy link
Contributor Author

cafce25 commented Oct 23, 2022

While you're at 1 maybe you can include a way the user can decide which colorscheme is used,
that way we don't have to decide (we can't anyways) what they can read.

@Shreya-7
Copy link
Contributor

@cafce25 Hmm, that makes sense. I can start looking into the basic asks here, and when I'm done I can try to implement this new functionality..

Couple more questions about it though:

  1. Are we talking about something like a dropdown on the reference page that the user can choose from?
  2. Would we allow the user to choose a scheme that would be applied globally or on every reference page? Say, if they want to use scheme1 when reading exception handling in Java, but scheme2 when reading classes in Ada versus choosing scheme1 on the homepage that gets applied across the board.
  3. Do we want to persist user preference so they do not have to select every time they visit the page? I don't think we support any customisations like that since we don't have sessions for a user. With a quick glance at the models we have, I can see we have something called SiteVisit but I'm not sure useful that would be.

@cafce25
Copy link
Contributor Author

cafce25 commented Oct 24, 2022

The main reason why I bring this up is because I don't think 1 can be solved by us alone.

  1. Are we talking about something like a dropdown on the reference page that the user can choose from?

I'd say anything that you come up with is fine.

  1. Would we allow the user to choose a scheme that would be applied globally or on every reference page?

I think supporting a single global setting is good enough.

  1. Do we want to persist user preference so they do not have to select every time they visit the page? I don't think we support any customisations like that since we don't have sessions for a user. With a quick glance at the models we have, I can see we have something called SiteVisit but I'm not sure useful that would be.

Definitely persist it, but since it's a local change that we don't need to know of or care for imo just putting it in their local store or saving it in a cookie should be fine.

@Shreya-7
Copy link
Contributor

Got it. I think I have all I need to get started.

@geekygirlsarah
Copy link
Member

I'll still watch over #641 but I'm wondering if this feature is starting to get a bit wild and outside of scope of something that makes a lot of sense to maintain. Pygments is designed to highlight really large segments of code and we don't really use them here. And I don't know that adding in a ton of settings for something so simple really seems worth the effort.

@geekygirlsarah geekygirlsarah removed the hacktoberfest Good issue for someone to work on for Hacktoberfest label Nov 10, 2022
@github-actions
Copy link

This issue has been inactive for 342 hours (14.25 days) and will be unassigned after 66 more hours (2.75 days). If you have questions, please leave a comment, message @codethesaurus or @geekygirlsarah on Twitter, or email coreteam@codethesaur.us.If you are still working on this issue, that's fine. Please comment here to tell the bot to give you more time.

@geekygirlsarah
Copy link
Member

I'm going to ask another friend or two what they think, so commenting to leave this open.

@github-actions
Copy link

This issue has been inactive for 346 hours (14.42 days) and will be unassigned after 62 more hours (2.58 days). If you have questions, please leave a comment, message @codethesaurus or @geekygirlsarah on Twitter, or email coreteam@codethesaur.us.If you are still working on this issue, that's fine. Please comment here to tell the bot to give you more time.

@geekygirlsarah
Copy link
Member

I'll comment to leave this open.

I closed the PR due to inactivity from @Shreya-7. I'm open to continuing discussions on this, but kind of thinking a solution might be dramatically more effort than is worth given how CT works.

@Shreya-7
Copy link
Contributor

Not sure what's waiting on me here, I thought we were getting second opinions from other folks? Do let me know if that's not the case or if I'm missing something!

@geekygirlsarah
Copy link
Member

There was more recent discussion on the PR than on here, and there were still open questions there that never got answered.

@Shreya-7
Copy link
Contributor

Got it, if the functionality on the PR seems a bit overkill maybe we can just replace the current colour scheme with a better one and call it a day?

The way I think the scheme is implemented in CT is that the CSS file was manually generated locally, referenced in the HTML and added to the repo. We can do the same for the new one that I had selected. Does that sound good?

@github-actions
Copy link

This issue has been inactive for 343 hours (14.29 days) and will be unassigned after 65 more hours (2.71 days). If you have questions, please leave a comment, message @codethesaurus or @geekygirlsarah on Twitter, or email coreteam@codethesaur.us.If you are still working on this issue, that's fine. Please comment here to tell the bot to give you more time.

@Chetan-Satpute
Copy link
Contributor

I tried using PrismJs for syntax highlighting (client side).

Here are two sample pages

It requires us to serve two static files

  • prism.js (~80K): including support for different programming languages.
  • prism.css (~3K): colorscheme (List of themes).

changes in application to implement this method

prismjs.com

@github-actions
Copy link

This issue has been inactive for 339 hours (14.13 days) and will be unassigned after 69 more hours (2.88 days). If you have questions, please leave a comment, message @codethesaurus or @geekygirlsarah on Twitter, or email coreteam@codethesaur.us.If you are still working on this issue, that's fine. Please comment here to tell the bot to give you more time.

@github-actions
Copy link

This issue has been inactive for 411 hours (17.13 days) and is past the limit of 408 hours (17.00 days) so is being unassigned.You’ve just been unassigned from this ticket due to inactivity – but feel free to pick it back up (or a new one!) in the future! Thank you for your interest in contributing to this project.

@aedwardg
Copy link
Contributor

Those prism examples above look like they work nicely with minimal changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Experience: Design good first issue A good issue for anyone new to open source to work on help wanted We'd love to have help working on this issue Type: Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants