Skip to content
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

Merged
merged 1 commit into from
May 10, 2022
Merged

Conversation

juans-chainsafe
Copy link
Contributor

closes #2121

@render
Copy link

render bot commented May 9, 2022

@render
Copy link

render bot commented May 9, 2022

@render
Copy link

render bot commented May 9, 2022

@github-actions github-actions bot added the Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes. label May 9, 2022
@juans-chainsafe juans-chainsafe self-assigned this May 9, 2022
@@ -337,6 +337,7 @@ const AppNav: React.FC<IAppNav> = ({ navOpen, setNavOpen }: IAppNav) => {

<div style={{ display: "flex" }}>
<Button
data-cy="button-sign-out"
Copy link
Member

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.

Copy link
Collaborator

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.

Copy link
Collaborator

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.

Copy link
Member

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

Copy link
Contributor

@FSM1 FSM1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@juans-chainsafe juans-chainsafe merged commit 58cdd65 into dev May 10, 2022
@juans-chainsafe juans-chainsafe deleted the mnt/add-sign-out-test-storage-2121 branch May 10, 2022 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Maintenance Added to issues and PRs when a change is for repository maintenance , such as CI or linter changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sign out test case in Storage
4 participants