-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Storage] Add sign out test #2122
Conversation
Your Render PR Server URL is https://chainsafe-components-stage-pr-2122.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c9shfs8nlkif2m5ucsk0. |
Your Render PR Server URL is https://files-ui-stage-pr-2122.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c9shft8nlkif2m5ucsog. |
Your Render PR Server URL is https://storage-ui-stage-pr-2122.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-c9shfu8nlkif2m5ucta0. |
@@ -337,6 +337,7 @@ const AppNav: React.FC<IAppNav> = ({ navOpen, setNavOpen }: IAppNav) => { | |||
|
|||
<div style={{ display: "flex" }}> | |||
<Button | |||
data-cy="button-sign-out" |
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 button component has a testId prop so instead of the custom tag data-cy tag you could also write
testId="sign-out"
and your locator would be [data-testid=button-sign-out]
It's a minor thing but I think when the component supports testId props it's probably better to use them as it's simpler, cleaner and makes sense to people reading the code for the first time. What do you think?
PS there may be other examples in the framework where we aren't using the available props as in some cases the component was updated after, we can clean those up as we encounter them.
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.
My 2cts here, testId
props should only be available for things where the data-cy
doesn't work. I think we should use the data-cy
as much as possible. To me, this is a mistake to have a testId
prop if the data-cy works. This is definitely something we should look into.
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.
what testId
does is that it passes down a data-testId
identifier to the underlying html component, if the html component isn't accessible from the code from Files. This is the case for some component from our library, but there are few afaik.
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.
Ok! Let's exclusively only use testId when regular data-cy doesn't work then, sorry for any confusion @juans-chainsafe
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
closes #2121