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

ActionBar: Add a few fixes and tests #4536

Merged
merged 7 commits into from
May 9, 2024
Merged

ActionBar: Add a few fixes and tests #4536

merged 7 commits into from
May 9, 2024

Conversation

pksjce
Copy link
Collaborator

@pksjce pksjce commented Apr 26, 2024

Closes # No particular issue

Changelog

  • Add the unsafeDisableTooltip prop for IconButton
  • Add proper aria-label in story
  • Add visual tests

@pksjce pksjce requested a review from a team as a code owner April 26, 2024 05:32
@pksjce pksjce requested a review from TylerJDev April 26, 2024 05:32
Copy link

changeset-bot bot commented Apr 26, 2024

🦋 Changeset detected

Latest commit: 7af6efc

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

github-actions bot commented Apr 26, 2024

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 88.24 KB (0%)
packages/react/dist/browser.umd.js 88.68 KB (0%)

})

// Default state
await page.getByText('Write').click()
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to press the "Write" tab for the default state? We might want to take the screenshot of the default state first without performing any action.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops my bad! we don't need to click it!

Copy link
Member

Choose a reason for hiding this comment

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

No worries, let me know when the snapshots are generated, I'll give another review 😊

@@ -33,3 +33,11 @@ describe('ActionBar', () => {
expect(results).toHaveNoViolations()
})
})

/* Test suite
1. Did it render all the iconbuttons?
Copy link
Member

Choose a reason for hiding this comment

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

I remember from UnderlineNav and it was easier to write those visual tests in Playwright than Jest but it is up to you 😊

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I think all the responsive stuff is better off tested in playwright. It's definitely harder(is it even possible?) to wrangle it in jest.
Will make a better division of test cases!

@@ -299,7 +299,7 @@ export const ActionBarIconButton = forwardRef((props: ActionBarIconButtonProps,
const domRect = (ref as MutableRefObject<HTMLElement>).current.getBoundingClientRect()
setChildrenWidth({text, width: domRect.width})
}, [ref, setChildrenWidth])
return <IconButton ref={ref} size={size} {...props} variant="invisible" />
return <IconButton ref={ref} size={size} {...props} variant="invisible" unsafeDisableTooltip={false} />
Copy link
Member

Choose a reason for hiding this comment

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

💖

@pksjce pksjce changed the title ActionBar: Add a few fixes and a test plan ActionBar: Add a few fixes and tests May 6, 2024
@pksjce pksjce requested a review from a team as a code owner May 7, 2024 04:50
@pksjce pksjce requested a review from tbenning May 7, 2024 04:50
@primer primer bot temporarily deployed to github-pages May 7, 2024 04:53 Inactive
@github-actions github-actions bot temporarily deployed to storybook-preview-4536 May 7, 2024 04:54 Inactive
@pksjce pksjce enabled auto-merge May 7, 2024 11:56
@pksjce pksjce removed the request for review from tbenning May 7, 2024 11:56
@pksjce pksjce disabled auto-merge May 7, 2024 11:57
@pksjce pksjce enabled auto-merge May 7, 2024 11:57
@pksjce pksjce added this pull request to the merge queue May 8, 2024
Merged via the queue into main with commit 024124a May 9, 2024
30 checks passed
@pksjce pksjce deleted the action-bar-fixes branch May 9, 2024 00:00
@primer primer bot mentioned this pull request May 8, 2024
JelloBagel pushed a commit that referenced this pull request May 16, 2024
* Add a few fixes and a test plan

* Add test for overflow

* Add overflow e2e test to actionbar

* Add tests

* Create mean-terms-bathe.md

* test(vrt): update snapshots

---------

Co-authored-by: pksjce <pksjce@users.noreply.github.com>
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.

4 participants