-
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
[Files] Change to dark theme and go back to light theme test #2162
Conversation
Your Render PR Server URL is https://chainsafe-components-stage-pr-2162.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cabnv8arrk0cgoep5hb0. |
Your Render PR Server URL is https://files-ui-stage-pr-2162.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cabnv9arrk0cgoep5hfg. |
Your Render PR Server URL is https://storage-ui-stage-pr-2162.onrender.com. Follow its progress at https://dashboard.render.com/static/srv-cabnvairrk0cgoep5i10. |
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 test checks whether the local storage string is set or not, but whether or not this actually reflect on the UI, and that it does switch to dark/light mode is not tested. I didn't look in details how to do this, but I'm pretty sure we could check things like the background color of a div for instance.
yes, you are right, and we do this validation as well: But we don't really check the UI (we though with Andrew of checking a background color but if that changes, the test will fail, but well, maybe those color hardly ever change) I will add that validation |
yup, if the test fails, then we'll fix it, it should be easy enough |
changes uploaded! I validated the change of color in the display header
|
@@ -24,6 +24,12 @@ export const settingsPage = { | |||
setUsernameButton: () => cy.get("[data-cy=button-set-username]"), | |||
usernamePresentInput: () => cy.get("[data-cy=input-profile-username-present]"), | |||
|
|||
// display tab | |||
displayTabButton: () => cy.get("[data-testid=tab-display]"), |
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.
Going by what @Tbaut mentioned here: #2122 (comment)
We should use data-cy instead of testid on elements like buttons and only use testid if data-cy doesn't work. Data-cy should work for this button.
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.
Sadly didn't work with data-cy :(
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.
Ah ok, well that's fair enough then. It's valid usage 👍
displayTabButton: () => cy.get("[data-testid=tab-display]"), | ||
displayTabHeader: () => cy.get("[data-cy=label-display-header]"), | ||
darkThemeLabel: () => cy.get("[data-testid=radio-input-dark-theme]"), | ||
lightThemeLabel: () => cy.get("[data-testid=radio-input-light-theme]"), |
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.
👍 These ones make sense though as I've never successfully got data-cy working with radio inputs
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.
Awesome Juan, nice to have test coverage for this functionality :)
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.
great one 🏴
closes #2141