Skip to content

Conversation

@lukewarlow
Copy link
Collaborator

This fixes WebIDL warnings

Fixes #693

This fixes WebIDL warnings
Copy link
Collaborator

@mfreed7 mfreed7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok to me, but I'm definitely not an expert in this area. I'll click Approve, but it'd be good to get someone more authoritative to review, maybe?

@hidde
Copy link
Contributor

hidde commented May 12, 2025

@lukewarlow would you be ok if I add the a11y-syntax-highlighting prism theme into this PR? It has colours that meet WCAG's colour criteria.

@lukewarlow
Copy link
Collaborator Author

Feel free to! The theme was picked somewhat at random iirc.

@hidde
Copy link
Contributor

hidde commented May 12, 2025

Great, have just PR'ed it towards your PR, hope that works?

Use accessible color scheme, support forced colors
@lukewarlow lukewarlow requested a review from keithamus as a code owner May 12, 2025 19:13
@lukewarlow
Copy link
Collaborator Author

Merged your PR into my PR branch.

@keithamus
Copy link
Collaborator

I'm somewhat supportive of this change, as I'm more familiar with Prism, but I'm not sure this really fixed #1169: it doesn't warn that webidl isn't supported, so it certainly suppresses the warnings but it also still doesn't support webidl as a syntax. In fact, looking through the site it looks as though some webidl code blocks which were correctly highlighting before are now no longer.

@lukewarlow
Copy link
Collaborator Author

Hmm it did support webidl? Perhaps Hidde's CSS changed that to some extent I can spend some time to look into that.

@hidde
Copy link
Contributor

hidde commented May 20, 2025

It should not have affected webidl (improved or worsened) as no selectors were changed, except for a few forced colors related ones. Will look into it today.

@hidde
Copy link
Contributor

hidde commented May 20, 2025

I may be looking in the wrong places, but when I compare instances of webidl on our site currently to in this PR, I see them rendered as plain text in the current site and as syntax highlighted in this PR?

  • focus group, block under feature detection heading (this PR, has highlight, existing, no highlight)
  • interest invokers, blocks under implementation details heading (this PR, has highlight, existing no highlight)
  • invokers, blocks under proposed plan heading (this PR, existing)
  • richer text fields, block under “how we'd like to solve” heading (this PR, existing)

@lukewarlow
Copy link
Collaborator Author

Yup looks correct to me. I'm assuming Keith looked locally and didn't install something correctly?

@hidde
Copy link
Contributor

hidde commented May 20, 2025

I do not see a package update in our PR, does syntaxHighlight: 'prism' magically install prism?

@lukewarlow
Copy link
Collaborator Author

Yeah astro supports both out of the box.

@keithamus
Copy link
Collaborator

Hm, that’s weird. I see the highlights now, whereas yesterday I swear I couldn’t.

In that case let’s merge this!

@keithamus keithamus merged commit 90dfb13 into openui:main May 20, 2025
5 checks passed
@lukewarlow lukewarlow deleted the prism branch May 20, 2025 08:48
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.

WebIDL highighting is missing

4 participants