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

querySelector should escape ids #42

Merged
merged 2 commits into from
Jan 6, 2024

Conversation

joshferrell
Copy link
Contributor

When using React.useId() the ID generated by the library looks something like ":S1:" which breaks virtual guidepup as the querySelector does not escape the id. This change simply wraps the id in CSS.escape

Referenced React issue:
facebook/react#26839

@cmorten
Copy link
Contributor

cmorten commented Jan 5, 2024

TIL!

Given we’re selecting an id only here with no plans for more complex selectors, I suspect getElementById() may be preferred as presumably requires processing to escape the string? (Per facebook/react#26839 (comment)) didn't think that through - getElementById() doesn't exist on HTMLElement and applying to the document wouldn't give us the child guarantee that we expect from this method, so what you've suggested is likely best!

It would be great to add a test in for this as well.

* Introducing a fragment introduced edge-case which wasn't covered, so extra null guard added
* For some reason linter/ts complaining so some fixups for that as well
* Upped timeouts as my old laptop was struggling to run tests in time!
@cmorten cmorten merged commit be3c0f7 into guidepup:main Jan 6, 2024
4 checks passed
@joshferrell
Copy link
Contributor Author

Thanks for updating the commit! I wasn't able to get around to adding the tests as quickly as I hoped.

@cmorten
Copy link
Contributor

cmorten commented Jan 7, 2024

Thanks for contributing! Sorry for treading on toes, had a free spell and thought would add on as had some other bits planned anyway 😅

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.

2 participants