-
Notifications
You must be signed in to change notification settings - Fork 331
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
Automated screenshots for Parabol demo #3907
Conversation
Hello @NikAiyer Implicitly, all code changes are subject to review, but the minimum requirement for reviewing a PR is to take steps provided by the PR author to get to the goal state. The ‘tests’ for the reviewer are not to be confused with actual tests in code. These can be as simple as the steps for a user to try a new feature and see that it’s producing the desired goal state (e.g. 1) start a check-in meeting 2) pin an agenda item 3) after the meeting is over, the pinned items persist. In this case, it might look like steps to run cypress locally and review the demo screenshots that are produced, probably just: Tests
Also, you could add specific items that you want reviewed e.g. a new pattern in code, a database migration, etc. |
Sorry I'll add that info to the pull request. |
@NikAiyer no worries at all, it’s just helpful process to the team and the reviewer — having explicit, clear items to review and steps to check, thanks for all the attention here! |
…nd replying to tasks and comments as well as sumamry page
Hey @NikAiyer I’m running this locally, but for some reason I’m not seeing generated screenshots. I’ve tried the Cypress interface and |
Hey @ackernaut, are all the tests running? Is it skipping over the demo tests? |
Hello again, it’s working now that I’ve pulled your latest commits. |
Feedback here:
|
…te account section for screenshot
Thanks for the feedback! I think I've got most of the suggested changes working correctly. I think there might be a bug regarding the summary stats section so I'll try to fix that as well. |
…owing up by restoring localstorage between tests
Hey @ackernaut, I was able to figure out that issue with the summary stats in Cypress. I believe everything is now properly working. |
hey @NikAiyer apologies for the delay here, I’ll give it another look today. Thanks for addressing the feedback! |
These are looking good! Nice work! (Edit: see takeaways at the end of this comment) ReflectAny of these Reflect screenshots look good (here showing GroupI especially like VoteI like DiscussI really like Meeting SummaryThe meeting summary image in the Discuss folder turned out well. I like that you removed the sign-up CTA section from the demo summary. It’s probably long to stick in an evergreen 101 article. I wonder if you could capture a 2nd image in a 1280x720 browser window where you only see the top portion and the first bit of the first user’s section. Takeaways
|
Hey @ackernaut, thanks for reviewing my changes!
|
I’m not sure this would live in our main README for the repository. Is it safe to start a README in Re: lobby and icebreaker images, I don’t want to blow up the scope of this work. I’ll let you judge if you want to shoot for that stretch goal. Also, I seem to recall us trying to minimize the instances of Cypress properties in the actual app components. I’m not up to speed on what the minimal need is there, as I assume sometimes it’s necessary. Was there a rule of thumb that you and Matt agreed on? |
I realize you probably aren’t able to merge, so let me know what additional work you want to do here, and I can look at this again when you’re ready. Thanks! |
Hmm, now that you mention it, it might be better to have that in a separate pull request. Just to keep things simpler. I'll get back to that when I finish the other issues I'm working on.
There wasn't an exact rule regarding this. Basically he told me to try to limit the amount of Cypress data-cy properties I used whenever I could. Here's what he said:
|
This overall LGTM! Agree with all of @ackernaut's feedback. Some general feedback:
As someone who doesn't currently have to the repo setup locally, I'd value a Loom video & Notion doc with instructions :) Is there any way to ensure more consistency in the images, so we're not needing to pull multiple times? Happy to go with this route and then see if the tension builds.
Is it possible to have both options always created for all screenshots? We'd potentially use those types of images differently. |
@NikAiyer Re: separate PR, I like it! Break the work down into future pieces. Re: data-cy props, thanks for sharing the extra context from your conversation with Matt, sounds good! @avivapinchas Thanks for the extra input here. Re: setting up the repo, we could probably break this down into 2 parts and make it as friendly as possible for folks who are going wider over deeper role-wise. 1) set up the repo 2) [do the thing locally you need—in this case running tests for screenshots] +1 for Loom and Notion. I can help with this once we get if farther. Re: sidebar open and closed, I think that @NikAiyer did include both. There are more images than what I shared as output from the tests, and they are named as different actions representing different movements in the linear progression of the meeting. Here’s a zip of the last batch I ran, shared from my drive. |
…nshots, added multiple reflections in group for discussion phase, changed filenames of screenshots to include parabol-retrospective, and included screenshots for when sidebar is open vs closed for all screenshots
@avivapinchas Thanks for the feedback! I believe I was able to get everything you wanted into the testing. For the consistency of the screenshots, I was able to get most of them to a consistent level. The only one I'm sort of iffy about is the Group Phase, which might need some tweaks to get better images. |
Looking now! On Friday, it would hang up when trying to pull latest (after the fact, I now wonder if this was due to using a VPN). Today it’s working, but my VPN is off. |
@NikAiyer this is looking good!!
Here’s the batch that was generated (shared zip via Google Drive) |
Hmm yeah I think that might be due to a timing thing. Maybe it doesn't get voted on in time so topics don't show up. I'll add a longer wait there to ensure it gets done. Edit: Looks like I forgot to reload the page before I started taking pictures. Sometimes it'll render the page before the cache is restored. |
…ary and changed a test name in demo_reflect
This pull request also fixes issue #3799, specifically the issue with some tests failing in CI. |
@NikAiyer looks like that worked as the demo summary now has data. The shots are looking good. There’s one annoying detail with the vote shots. Most of them have a visible scrollbar. Do you know what triggers the scrollbar? Is it the timing after the vote is made? |
I think that happens when there are a lot of reflections in one category. I'll let them get grouped a little bit so that doesn't show up. |
… removing scrollbar when taking screenshots in vote phase
Good to know, timing sure is nuanced. Thanks for the attention to these details. I’m looking again locally. |
👏 thanks @NikAiyer for the many iterations, merging now! |
This pull request attempts to resolve #3767 and uses the cy.screenshot() command to take pictures of the demo while it is running.
Steps to get running: