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(tag): use refs to handle component access #16571

Merged

Conversation

Kilian-Collender
Copy link
Contributor

@Kilian-Collender Kilian-Collender commented May 24, 2024

Closes #16570

When using the querySelector it is easily broken if the id has reserved characters also the isEllipsisActive helper had no protection for a non element. So now using refs to access the DOM element and the helper has been extracted for reuse and includes extra protection.

Changelog

New

  • {{new thing}}

Changed

  • Update how the tag components reference themselves

Removed

  • {{removed thing}}

Testing / Reviewing

  • Follow the example in the original bug to test that the original issue does not take place.

N.B. There was no testing in place around the recent changes that were added, it is strongly advised that we need tests here both unit and E2E.

When using the querySelector it is easily broken if
the id has reserved characters.
Also the isEllipsisActive helper had no protection
for a non element.
@Kilian-Collender Kilian-Collender requested a review from a team as a code owner May 24, 2024 13:25
@guidari guidari self-requested a review May 24, 2024 13:26
Copy link

netlify bot commented May 24, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 1f52e43
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6650b43acade9600085f88ac
😎 Deploy Preview https://deploy-preview-16571--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented May 24, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1f52e43
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6650b43a7d75f7000880aa1a
😎 Deploy Preview https://deploy-preview-16571--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

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

Thanks again for the quick fix!

@tay1orjones
Copy link
Member

Hey real quick before we merge this I'm going to correct the deprecation notices

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@guidari
Copy link
Contributor

guidari commented May 24, 2024

Thanks for that! I added a forwardRef to Tag, so the ref can use in the variants. If the CI pass I think we are good to merge it!

return (
<ComponentTag className={tagClasses} id={tagId} {...other}>
<ComponentTag ref={tagRef} className={tagClasses} id={tagId} {...other}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should this not be ref too now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually we can remove the ref from this one. The filter prop should be replace with the new variant Dissmisible Tag so we don't have to add the new functionality to the old filter tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but isn't the Dismissible Tag experimental? so you still need it there until it is fully deprecated

Copy link
Contributor

@guidari guidari May 24, 2024

Choose a reason for hiding this comment

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

I see your point. It is something I will take to the team to talk about it. Since we have implemented in the Dissmisible Tag it would be quick to implement on the old filter prop.
But for now the spec we have is only in the new Intereactive Tag.

@guidari guidari requested a review from a team as a code owner May 24, 2024 14:41
@tay1orjones tay1orjones added this pull request to the merge queue May 24, 2024
@tay1orjones tay1orjones removed this pull request from the merge queue due to a manual request May 24, 2024
@tay1orjones tay1orjones merged commit df8c92a into carbon-design-system:main May 24, 2024
21 checks passed
@Kilian-Collender Kilian-Collender deleted the kc-tag-bug-16570 branch May 24, 2024 16:25
tay1orjones added a commit to tay1orjones/carbon that referenced this pull request May 24, 2024
…6571)

      * fix(tag): use refs to handle component access

When using the querySelector it is easily broken if
the id has reserved characters.
Also the isEllipsisActive helper had no protection
for a non element.

* fix(tag): cast BaseComponent type

* fix(tag): improve deprecation notices

* fix: added forwardRef to Tag to grab ref in variants

* fix: removed console log

* fix: fixed spelling and remove ref from old filter

* fix: updated snapshots

* fix: fixed onMouseEnter error on console

* fix: fixed TS error

---------

Co-authored-by: Taylor Jones <[email protected]>
Co-authored-by: guidari <[email protected]>
@tay1orjones tay1orjones mentioned this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Tag components use query selector with id value without protection
4 participants