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

fix(editor): prefer useId when available #461

Merged
merged 2 commits into from
May 25, 2022
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented May 6, 2022

Haven't actually tested but we need to do this. I see mismatches otherwise.

I tried to keep it backwards-compatible. If you drop 17 support then you can just useId.

@@ -125,7 +125,10 @@ export const CodeMirror = React.forwardRef<CodeMirrorRef, CodeMirrorProps>(

const c = useClasser("sp");
const { listen } = useSandpack();
const ariaId = React.useRef<string>(id ?? generateRandomId());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note sure if the "passed-in ID" use case is important.

Copy link
Member

Choose a reason for hiding this comment

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

Initially, the purpose of the id props is to get consistent ids in snapshot tests. But I guess useId will always return a constant value, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as you don't change the parent component path. If you do, the snapshot will update.

@danilowoz danilowoz changed the title Prefer useId when available fix(editor): prefer useId when available May 16, 2022
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 76fc29a:

Sandbox Source
Sandpack Blog Example 1 Configuration

@danilowoz danilowoz merged commit a7187db into codesandbox:main May 25, 2022
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