Syntax Highlighting Capabilities #1574
Conversation
|
Also I see Lighthouse is complaining as we have some invisible text so might need to look at the CSS we've chosen for these styles.
P.S. I love our automated testing to catch these things!! 🎉 |
|
@bazzadp, Changes are done as per Review |
|
Looking good from a brief scan of the code! Will spend some time testing this today and let you know how I get on. One comment so far is that the Methodology section now uses different Syntax highlighting for its SQL snippet. We should move that to Rainbow format, and remove its custom code CSS, so we don’t need to maintain two and so we can auto-generate it in future. But let’s merge this first and then look at that separately as there are quite a lot of Methodology files to change with all the various translations, so this initial PR for the chapters is easier to read and review. |
…ax_highlighting code improved
|
I haven't taken a close look at this PR but one request I have is to try to keep the syntax highlighting color scheme as close to the manual one used on the Methodology page. Those colors were chosen by the site designer to match our overall palette, so it'd be great to continue to use those for consistency. Do we have the ability to override the default theme colors of the syntax highlighter tool? |
|
Yup. The syntax highlighter tool is merely used to apply the HTML spans with classes for each supported language (JS, HTML, CSS and SQL). The CSS for those classes is in 2019.css (though my understanding is it was based on one of the suggested styles from the tool). @bharatagsrwal could you look at the Methodology page and see what changes we need to do to bring this inline? @rviscomi as I mentioned above, after we merge this we’ll look at changing the Methodology chapter to use the same spans and classes and remove the CSS from that page so it uses the same one. That should result in no visual change to Methodology page but consistent code. But as there are a lot of Methodology pages will land this first, and then do that next. Maybe as part of that we should make the SQL a function call to avoid repeating it so much in eat translation even though it’s not translated? Anyway, as I say, let’s land this first. |
|
okay, we just need to change the colors in CSS and we are good to do, so I will change those and make it the same as in the Methodology page |
|
How about these? I have changed to those colors which are on the methodology page |
@rviscomi what do you think about something else for strings? Something like #e74c3c ?
Without making this a least a bit more different you kind of lose the benefit of having syntax highlighting! We don't have any string syntax highlighting in Methodology, which is why it's subtlety have not been noticed before, but comments and numbers are more visually different, so I think strings should be too:
|
|
+1 for more contrast, that was my first impression of #1574 (comment) as well. The table name in the query (with backticks) is also technically a string if we wanted to keep it consistent. |
yes, but it seems like it is not a string, the color is almost the same as the default color, so I think @bazzadp #e74c3c this color can be a good option for strings |
Looks like the Rainbow library we're using is clever enough to pick this up! This is what the SQL looks like when run through that:
Though interesting enough it doesn't recognise |
you did by Firstly converting using rainbow and pasting code in HTML file? |
Nope. I copied the raw SQL from rendered Methodology page and then temporarily put it in the capabilities chapter page in a sql block and generated the chapter to see what it would look like. Did it perfectly (apart from that comment) first time. So yeah basically the same :-) Will submit a PR to rainbow to add |
|
PR to add @bharatagsrwal you happy to accept those two small changes I suggested so we can merge this? |
Yap, I am doing the changes, so what color should be for String, is it that red one or the same as of now? |
|
when I tried to change color to #e74c3c |
|
Orange is always a tough one. #B31107 is the best I can find after plugging it into: https://contrast-finder.tanaguru.com/result.html?foreground=%23FFFFFF&background=%23e74c3c&ratio=7&isBackgroundTested=true&algo=Rgb&distanceSort=asc |
looks good |
|
Please note, that the following diffs happen in the templates on this branch compared to main: |
|
@bharatagsrwal just realised you're not in the contributors json. Could you submit a PR with your details adding yourself to the |
Syntax Highlighting Capabilities for HTML, CSS, JS and SQL added
please check
generate_chapters.js,2019.cssandpackage.jsonfile.