Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Testing: Add an e2e test to check the interactions in write mode #65819
Testing: Add an e2e test to check the interactions in write mode #65819
Changes from 1 commit
3265fdf
f158e0e
2bded86
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Nit: Prefer using unions of string literals in types since we only have two options for now. Something like
'Design' | 'Write'
.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'm hesitant, someone could decide to test another language.
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.
They're only types so I guess they can always bypass that. We can also add
| string
to the end to allow arbitrary strings.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.
Using class name selectors is not recommended. Could we instead select it by
getByRole
and optionally with alevel
field here?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 know but for me, this is not easy to pinpoint and avoid random failures when we add headings... I prefer the selector here.
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 there's already a heading in place? What do you mean by pinpointing random failures? I'd appreciate if we could follow the established best practices whenever possible and add comments if we could not. Thanks! 🙏
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.
The
h2
is not enough to say that I'm targeting the "title" of the inspector panel. There could be more h2 in the inspector panel, in fact there are already. (content).We could theoretically say
h2 > eq( 0 )
but that's not very stable, any random design change can break that. (moving titles around).So for me, in this case, I prefer a non ambiguous assertion rather than a generic one.
I agree on following best practices, but we need to understand what's important and why they exist.
I'm happy to update If there's a less ambiguous approach using that doesn't use selectors.
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.
A common workaround practice throughout the codebase is using
expect( getByRole('heading', {name: 'Group', level: 2}) ).toBeVisible()
.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'd echo Riad about until fixing the a11y issue, we should stick with that classname for now and unblock this PR. Let's create an issue for this.
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.
No need to block this PR. But we should raise a followup Issue as suggested with the appropriate labels to get Accessibility feedback.
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.
IMO,
getByRole
is always preferred, in cases when it doesn't apply, we should still fallback to alternatives likegetByTestId
than class name selectors. I don't understand the argument of using class name selectors being more "solid". TBH, this has happened too many times that this is becoming frustrating to me. Could we at least add a comment above indicating that this is an edge case so that people won't accidentally follow the practice? I don't think it should block this PR, but I think this type of issue is still important, especially given that it just flagged a potential accessibility concern here by trying to avoid the class name selectors.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'll add a comment. I'm not sure we used test ids before.
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.
Thanks for the comment.
Nit: the wording isn't quite accurate and should read (emphasis added for clarity):