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(components): There was a typo in the DataList.ItemAction logic #2395

Merged
merged 2 commits into from
Feb 24, 2025

Conversation

itsleaheye
Copy link
Contributor

@itsleaheye itsleaheye commented Feb 24, 2025

Motivations

I merged a typo with my branch last week that prevents the persistent ItemActions from rendering correctly. For shame.

Fixed

Screenshot 2025-02-24 at 13 57 29
  • Fix spelling so that the DataList.ItemAction properly renders

Testing

  • Open docs/components/DataList/Web.stories.tsx and set one of the item actions to alwaysVisible={true}, delete the other item actions
  • Navigate to your local story book and verify you no longer see the 'more' icon and instead see your item actions label on hover of the row.
Screenshot 2025-02-24 at 13 50 52

In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

Copy link

cloudflare-workers-and-pages bot commented Feb 24, 2025

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 0911d11
Status: ✅  Deploy successful!
Preview URL: https://d66a6818.atlantis.pages.dev
Branch Preview URL: https://job-114615-job-116871-shame.atlantis.pages.dev

View logs

@MichaelParadis MichaelParadis self-requested a review February 24, 2025 20:54
@@ -46,7 +46,7 @@ export function DataListActions<T extends DataListObject>({
const actionLabel = getActionLabel();

// If the action is always visible, we don't want a tooltip.
if (props.alwaysVisibled) {
if (props.alwaysVisible) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

200w

Copy link
Contributor

Choose a reason for hiding this comment

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

I am curious about how CI didn't catch this when compiling the typescript 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my thought. It should have flagged it as an undefined var. I'll look at adding an additional unit test

Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to figure out why. Because the of the way children for the actions are retrieved the type is any so it can't actually know.

Thanks for adding the test, let me know when that is done and I can merge this and get it into the version bump

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are added. Just waiting on CI 😎

Copy link
Contributor

@MichaelParadis MichaelParadis left a comment

Choose a reason for hiding this comment

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

LGTM (pending CI)! I will merge once CI finishes

@MichaelParadis MichaelParadis merged commit fc30eb4 into master Feb 24, 2025
12 checks passed
@MichaelParadis MichaelParadis deleted the JOB-114615/JOB-116871/shame-leah branch February 24, 2025 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants