Uh oh!
There was an error while loading. Please reload this page.
Switch to using useId() for generated CSS class names#68
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
Co-authored-by: weotch <77567+weotch@users.noreply.github.com>
| dataAttributes, | ||
| }: VisualWrapperProps): ReactNode { | ||
| // Generate unique ID for CSS classes when needed | ||
| const baseId = useId(); |
There was a problem hiding this comment.
Can you changes instances of baseId to cssId for more clarity on what this id is used for
There was a problem hiding this comment.
Changed all instances of baseId to cssId for better clarity on what this ID is used for. Commit 6967131.
Co-authored-by: weotch <77567+weotch@users.noreply.github.com>
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
weotch
left a comment
There was a problem hiding this comment.
I don't think we need to generate multiple cssClasses anymore. Can you lift the creation of the cssClass up into VisualWrapper? Like I think it'll be something like this:
constresponsiveClassname=useId().replace(/:/g,"")And then responsiveClassname gets passed into makeResponsiveAspects and is used by all the cssRules that get generated. Thus, makeResponsiveAspects only needs to return the completed aspectStyleTag.
…queries Co-authored-by: weotch <77567+weotch@users.noreply.github.com>
... Implemented your suggestion to use a single CSS class name for all media queries. The Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR updates VisualWrapper’s responsive-aspect CSS generation to use a useId()-derived class name, simplifying the generated CSS selectors and removing the prior media-query/aspect-derived class naming scheme.
Changes:
- Use React
useId()to generate a per-component CSS class used for responsive aspect-ratio rules. - Refactor
makeResponsiveAspectsto return only a<style>tag containing@mediarules targeting a shared class. - Add a Cypress component test intended to verify the new class-name behavior and that legacy
rv-*class formats are not produced.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/VisualWrapper.tsx | Switch responsive aspect CSS selector generation to a useId()-based class and simplify makeResponsiveAspects output. |
| packages/react/cypress/component/VisualWrapper.cy.tsx | Add a Cypress test to assert new class-name behavior and presence of responsive aspect CSS rules. |
Suppressed comments (1)
packages/react/cypress/component/VisualWrapper.cy.tsx:108
cy.get("style")may match unrelated style tags injected by the test harness or other mounted components, making this assertion flaky. SinceVisualWrapperrenders the<style>inside the wrapper, scope the query to.wrapper.
// Verify that a style tag was created with the responsive CSS
cy.get("style").should("exist").and("contain", "aspect-ratio");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if (typeof aspect == "function" && sourceMedia && sourceMedia.length) { | ||
| ({ aspectStyleTag, aspectClasses } = makeResponsiveAspects({ | ||
| aspectStyleTag = makeResponsiveAspects({ | ||
| aspectCalculator: aspect, | ||
| sourceMedia, | ||
| image, | ||
| video, | ||
| })); | ||
| responsiveClassname, | ||
| }); |
| // Generate unique CSS class name for responsive aspects | ||
| const responsiveClassname = useId().replace(/:/g, ""); | ||
| const responsiveClass = classes[0]; | ||
| expect(responsiveClass).to.not.be.empty; | ||
| expect(responsiveClass).to.not.start.with("rv-"); | ||
Replaces the bespoke CSS class name generation in
makeResponsiveAspectswith React'suseId()hook for better uniqueness and cleaner output.Changes Made
rv-orientation-landscape-2orrv-max-width-767px-1_333useId()hook with a simple index suffix, generating cleaner names liker1-0,r1-1, etc.Implementation Details
useIdimport toVisualWrapper.tsxVisualWrappercomponent to generate a unique base ID when responsive aspects are neededmakeResponsiveAspectsfunction to accept and use the base ID instead of generating class names from media query stringsMakeResponsiveAspectsPropstype to include thebaseIdparameterBenefits
The functionality remains exactly the same - only the CSS class name generation approach has changed.
Fixes#65.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
download.cypress.io/usr/local/bin/node index.js --exec install(dns block)http://168.63.129.16:80/machine//usr/bin/python3 -u bin/WALinuxAgent-2.13.1.1-py3.9.egg -collect-logs(http block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.