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.

Sign up for GitHub

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

Jump to bottom

Clipboard & Copy Button - Fix handling of empty string and zero values (HDS-4447) #2685

Merged
merged 9 commits into from
Feb 7, 2025
Merged

Clipboard & Copy Button - Fix handling of empty string and zero values (HDS-4447) #2685

merged 9 commits into from
Feb 7, 2025

Conversation

KristinLBradley
Copy link
Contributor

@KristinLBradley KristinLBradley commented Feb 4, 2025 •
edited
Loading

📌 Summary

If merged, this PR adds a fix to allow empty strings and zero number values passed into the Copy Button to copy the values.

🛠️ Detailed description

  • Update clipboard modifier used by Copy Button & Copy Snippet to handle passed in empty string and zero number values
  • Add Showcase examples
  • Add tests

🔗 External links

👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

@vercel Vercel
Copy link

vercel bot commented Feb 4, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Feb 6, 2025 7:38pm
hds-website ✅ Ready (Inspect) Visit Preview Feb 6, 2025 7:38pm

@KristinLBradley KristinLBradley marked this pull request as ready for review February 4, 2025 23:12
@KristinLBradley KristinLBradley requested a review from a team as a code owner February 4, 2025 23:12
didoo
didoo reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@didoo didoo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@KristinLBradley left a few comments; let me know if you want to discuss them in a call if it's not clear or doesn't makes sense to you what I'm suggesting

.changeset/early-knives-fly.md Show resolved Hide resolved
showcase/tests/integration/components/hds/copy/button/index-test.js Outdated Show resolved Hide resolved
showcase/tests/integration/components/hds/copy/button/index-test.js Outdated Show resolved Hide resolved
packages/components/src/modifiers/hds-clipboard.ts Outdated Show resolved Hide resolved
showcase/app/templates/components/copy/button.hbs Outdated Show resolved Hide resolved
@KristinLBradley
HDS-4447 Remove check for textToCopy in writeTextToClipboard function…
94b18eb 
… and remove test for passing undefined value
@KristinLBradley KristinLBradley requested a review from didoo February 5, 2025 20:13
didoo
didoo reviewed Feb 6, 2025
View reviewed changes
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

left a comment around missing showcase use cases for Copy::Snippet. Apart from that, everything is fine for me

showcase/app/templates/components/copy/button.hbs Show resolved Hide resolved
@KristinLBradley KristinLBradley requested review from didoo and a team February 6, 2025 17:42
didoo
didoo reviewed Feb 7, 2025
View reviewed changes
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Nice job! 👏

@KristinLBradley KristinLBradley merged commit 0361b9b into main Feb 7, 2025
14 checks passed
@KristinLBradley KristinLBradley deleted the hds-4447-copybutton-empty-string branch February 7, 2025 20:21
@hashibot-hds hashibot-hds mentioned this pull request Feb 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@didoo didoo didoo approved these changes

Assignees
No one assigned
Labels
packages/components showcase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KristinLBradley @didoo @hashibot-hds