-
Notifications
You must be signed in to change notification settings - Fork 142
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
test(ProductiveCard): add avt test coverage #6518
test(ProductiveCard): add avt test coverage #6518
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ibm-products-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6518 +/- ##
==========================================
- Coverage 79.97% 79.96% -0.01%
==========================================
Files 394 394
Lines 12894 12895 +1
Branches 4273 4274 +1
==========================================
Hits 10312 10312
- Misses 2582 2583 +1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I think there are a few places where we could also use role/text locators rather than by query selectors. :)
const buttons = page.locator(`button.${carbon.prefix}--btn`); | ||
const disabledButton = buttons.nth(2); | ||
expect(await disabledButton.getAttribute('disabled')).not.toBeNull(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we typically try to query by role or text over a specific query selector.
In this case, we could probably do
const disabledButton = page.getByRole('button', { name: 'Read more' });
Or some similar variation of that.
const menuButton = page.locator('button[aria-haspopup="true"]'); | ||
const menu = page.locator('ul[role="menu"]'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably use the roles here as well. :)
@elycheea fixed it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also agree it might be good to deprecate iconDescription
or overflowAriaLabel
if they are now redundant. If iconDescription
overrides overflowAriaLabel
, that may be preferable unless the latter is more widely used. 🤔
im pretty sure we can deprecate although a more generic name any additional thoughts or suggestions? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @devadula-nandan can you add test for hover state.
@anamikaanu96 done |
dd60a2e
Closes #6472
adds accessibility tests for productive card
Since we are in v12 OverflowMenu. i noticed that passing aria-label prop to v12 OverflowMenu spreads it to the overflow menu container which is a div and is reported as violation
v12 OverflowMenu uses single
Label
prop to set tooltip text on menu button and aria-label for list container (ul)which means we might need to deprecate either
iconDescription
oroverflowAriaLabel
asLabel
prop handles both with v12 OverflowMenu.useful references
#5803
#6395
i have passed label as below, and awaiting for any review suggestions
What did you change?
e2e/components/ProductiveCard/ProductiveCard-test.avt.e2e.js
packages/ibm-products/src/components/Card/Card.tsx
How did you test and verify your work?
yarn avt