-
Notifications
You must be signed in to change notification settings - Fork 364
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
Add data-test-id to example pages for regression #881
Conversation
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.
Just have one question I'd like answered in the menu button kb documentation before merging.
@@ -118,11 +118,11 @@ <h3 id="kbd1_label">Menu Button</h3> | |||
</tr> | |||
</thead> | |||
<tbody> | |||
<tr> | |||
<tr data-test-id="button-down-arrow"> |
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 are 3 keys to test here. Should that somehow be represented with the testid value? Multiple values? Multiple IDs?
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.
in previous instances I used values like key-enter-or-space
. This scenario, I simply had two tests for this data-test-id, one that tested enter
and one that tested space
. The value button-down-arrow-or-space-or-enter
is a little long, but I could continue that convention (I would have but I missed the multiple entries in this row).
Alternatively, the data-test-ids could be space delimited values. This would require going back and changing a few data-test-ids and test files. I would prefer to do this after you merge to master so I don't have to update things that depend on each other in two different branches? For now I'll use the old convention, if you don't mind!
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 fine with the old convention for now but think multiple space-delimited tokens is probably a better long term approach. It would be good for each test to have its own token so that if we change a key, e.g., split to a different row, we'd only have to change the test for that key and it wouldn't effect testing the other key presses that didn't change.
ok with this solution for now, @mcking65 ? |
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.
LGTM
Regression testing: Add data-test-id to example pages (pull #881)
@mcking65, please review and merge when convenient :)