Percy: Introduce hide-in-percy and hide diff problematic elements #3689
Conversation
| @@ -36,14 +41,6 @@ describe('Edit Profile', () => { | |||
| }); | |||
There was a problem hiding this comment.
Perhaps return the original "secret" api key at the end of the test so that test ordering wouldn't matter?
There was a problem hiding this comment.
IMO setting the state after the test is not a good approach as well. My ideas to improve it are:
- try to intercept API response and modify "api_key" from it (it isn't as easy as I initially thought ^^);
- modify the
edit_profile_specto do something I should've done at first, but leaned later: create a user to be tested prior the tests instead of asserting on the Admin user (this should fix this one).
The goal here is to get some time to do the above without annoyance in Percy. LMK what you think 🙂
There was a problem hiding this comment.
The best approach IMO is to actually "blackout" the element so it doesn't get included in screenshots.
@media only percy {
.hide-in-percy {
visibility: hidden;
}
}There was a problem hiding this comment.
@gabrieldutra if you go this route, can you piggyback pace as well (it's causing Percy false negatives)
There was a problem hiding this comment.
Not sure this is the very best solution to those cases as we are not capturing changes to that part of the UI. However this seems the best for now and I wanted to introduce Percy specific css anyway 🙂.
@ranbena please leave your comments, when you think this is ok I'll rename the PR and merge it.
.circleci/config.yml
Outdated
| @@ -84,6 +84,8 @@ jobs: | |||
| command: | | |||
| npm run cypress start | |||
| docker-compose run cypress npm run cypress db-seed | |||
| # Make sure the API key is the same so Percy snapshots are consistent | |||
| docker-compose -p cypress run postgres psql -U postgres -h postgres -c "update users set api_key = 'secret' where email ='admin@redash.io';" | |||
There was a problem hiding this comment.
Can you instead make this be able to be set from db seeding?
{
route: '/setup',
type: 'form',
data: {
name: 'Example Admin',
email: 'admin@redash.io',
password: 'password',
org_name: 'Redash',
api_key: 'secret', // <- HERE
},
},There was a problem hiding this comment.
I don't like very much the idea of changing the API to this, in any case, please check my ideas in the other comment and LMK what you think.
client/app/assets/less/inc/misc.less
Outdated
| -----------------------------------------------------------*/ | ||
| @media only percy { | ||
| .hide-in-percy, .pace { | ||
| display: none; |
There was a problem hiding this comment.
display: none; seemed better, but tell me if you think we get some value on having a white space i.e.: to know it's hidden or for alignment purposes (this last one seems important, but we can create another class when it becomes necessary)
There was a problem hiding this comment.
There's no difference to Percy comparison.
But visibility keeps the layout unaltered while display might mess things up, which might confuse a human looking at the images. So I favor visibility: hidden.
You can even go as far as this, to have full clarity 😬 (only if you want to):
@media only percy {
.hide-in-percy, .pace {
visibility: hidden;
position: relative;
&:after {
content: 'Hidden from Percy';
font-size: 10px;
color: #000;
opacity: 0.5;
visibility: visible;
position: absolute;
left: 2px;
top: 2px;
}
}
}There was a problem hiding this comment.
You can even go as far as this, to have full clarity grimacing (only if you want to)
Nah, this may create issues when we want to hide small things or when the object will eventually disappear from the DOM.
|
Next step is certainly discover a fix to those square icons. They are even more annoying than Pace 😆 |
| @@ -176,7 +176,7 @@ export default class UserEdit extends React.Component { | |||
| const { user, regeneratingApiKey } = this.state; | |||
|
|
|||
| return ( | |||
| <Form layout="vertical"> | |||
| <Form className="hide-in-percy" layout="vertical"> | |||
There was a problem hiding this comment.
I think it's important to hide only the precise offending content <InputWithCopy>.
gabrieldutra
changed the title
What type of PR is this? (check all applicable)
Description
This introduces the
.hide-in-percyclass to hide elements in the Page on Percy Screenshots and hide the following elements:Related Tickets & Documents
Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Check Percy diff 🙂