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: WT-1750 Patch DOM ID clashing #918

Merged
merged 8 commits into from
Oct 3, 2023
Merged

fix: WT-1750 Patch DOM ID clashing #918

merged 8 commits into from
Oct 3, 2023

Conversation

andrearampin
Copy link
Contributor

@andrearampin andrearampin commented Sep 28, 2023

Summary

BIOME and Checkout used to have components with the same ID making it not possible for BIOME to correctly target the some DOM elements.

https://immutable.atlassian.net/browse/WT-1750

Screenshot 2023-09-29 at 5 02 32 pm
Screen.Recording.2023-10-03.at.3.22.04.pm.mov

Why the changes

Bug when multiple widgets are rendered on a web page

Things worth calling out

Before submitting the PR, please consider the following:

  • Prefix your PR title with feat: , fix: , chore: , docs:, or refactor:.

@andrearampin andrearampin requested review from a team as code owners September 28, 2023 07:16
@@ -7,7 +7,7 @@ import { CoinSelector } from '../../CoinSelector/CoinSelector';
import { CoinSelectorOptionProps } from '../../CoinSelector/CoinSelectorOption';

interface SelectFormProps {
id: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was used for testing -- move it to testId to prevent DOM id clashing

@@ -9,7 +9,7 @@ import { TextInputForm } from '../TextInputForm/TextInputForm';
import { CoinSelectorOptionProps } from '../../CoinSelector/CoinSelectorOption';

interface SelectInputProps {
id: string;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was used for testing -- move it to testId to prevent DOM id clashing

Copy link
Contributor

@dreamoftrees dreamoftrees left a comment

Choose a reason for hiding this comment

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

Nice clean up @andrearampin 👍 Just a minor question around the actual use of id across the Box elements.

Copy link
Contributor

@dreamoftrees dreamoftrees left a comment

Choose a reason for hiding this comment

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

Nice work @andrearampin

@andrearampin andrearampin enabled auto-merge (squash) October 3, 2023 08:41
@andrearampin andrearampin merged commit f8edcc7 into main Oct 3, 2023
6 checks passed
@andrearampin andrearampin deleted the WT-1750 branch October 3, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants