-
Notifications
You must be signed in to change notification settings - Fork 156
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
feat: Adds test id to flashbar dom selector #2955
feat: Adds test id to flashbar dom selector #2955
Conversation
7aff7c9
to
2e88cbb
Compare
@@ -13,6 +13,7 @@ exports[`test-utils selectors 1`] = ` | |||
], | |||
"anchor-navigation": [ | |||
"awsui_anchor-item--active_17oho", | |||
"awsui_anchor-item_17oho", |
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.
This is residual snapshot change from #2855.
Since we are merging to the project branch feat/test-utils-project-m2
rather than main
, this wasn't caught by the Github checks.
You can ignore this one.
@@ -72,7 +72,7 @@ export const focusFlashById = throttle( | |||
{ trailing: false } | |||
); | |||
|
|||
export interface FlashProps extends FlashbarProps.MessageDefinition { | |||
export interface FlashProps extends Omit<FlashbarProps.MessageDefinition, 'testId'> { |
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.
There's a bit of an inconsistency when it comes to FlashWrapper
.
FlashWrapper
should point to the list item and not the flash component itself:
static rootSelector: string = styles['flash-list-item']; |
But the existing findFlashByType
returns the flash rather than the parent <li />
:
return this.findAll(`.${styles['flash-list-item']} .${styles[`flash-type-${type}`]}`).map( |
To make sure than this mistake doesn't happen with testId
I omitted that from the FlashProps
so that it is not assigned by mistake in the future.
src/test-utils/dom/flashbar/index.ts
Outdated
* @returns {FlashbarWrapper | null} | ||
*/ | ||
findItemByTestId(testId: string): FlashWrapper | null { | ||
return this.findComponent(`.${styles['flash-list-item']}[data-testid="${CSS.escape(testId)}"]`, FlashWrapper); |
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.
Existing similar places use escapeSelector
from test-utils (https://github.com/cloudscape-design/test-utils/blob/f49a5a90ce077253f74d82659f1cb6e83092a2a4/src/core/utils.ts#L7), which ensures that this is polyfilled. Might be good to use that here too for consistency
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.
Good point! Replaced it. Also replaced one more occurrence that was using CSS.escape
from before!
2e88cbb
to
7644234
Compare
Description
Adds
testId
prop to flash items inside the flashbar component.Related links, issue #, if available:
Collective PR: #2985
Project: Improving test utils API
How has this been tested?
Tests added to cover the change.
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md
.CONTRIBUTING.md
.Security
checkSafeUrl
function.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.