Skip to content

✅ [AMP Story] [Page attachments] Update visual tests for new UIs #34223

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 33 commits into from
May 24, 2021
Merged

✅ [AMP Story] [Page attachments] Update visual tests for new UIs #34223

merged 33 commits into from
May 24, 2021

Conversation

raxsha
Copy link
Contributor

@raxsha raxsha commented May 5, 2021 •
edited
Loading

Adding visual tests for:

Inline

  • default
  • dark theme
  • custom text
  • 1 image
  • 2 images

Outlink

  • default
  • dark theme
  • custom theme background
  • custom theme text color
  • no icon
  • custom text

Demo:
visualdifftests

Fixes #33185.
Fixes #33184

@raxsha raxsha requested a review from processprocess May 5, 2021 02:39
@raxsha raxsha changed the title [AMP Story] [Page attachments] Update visual tests for new UIs ✅ [AMP Story] [Page attachments] Update visual tests for new UIs May 5, 2021
@processprocess
Copy link
Contributor

Also fixes #33184 :)

processprocess
processprocess reviewed May 5, 2021
View reviewed changes
Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Looks like the visual-tests part still needs to be written but the html template is looking good! Left some comments.

@raxsha raxsha requested a review from processprocess May 5, 2021 16:33
@raxsha raxsha requested a review from estherkim May 5, 2021 17:50
@raxsha raxsha requested a review from gmajoulet May 5, 2021 20:37
processprocess
processprocess previously requested changes May 6, 2021
View reviewed changes
test/visual-diff/visual-tests Outdated
Comment on lines 883 to 884
"url": "examples/visual-tests/amp-story/amp-story-page-attachment-v2.html",
"name": "amp-story-page-attachment-v2: displays new inline & outlink CTA buttons with various customizations",
Copy link
Contributor

@processprocess processprocess May 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Nice, but each test is for one snapshot.
We need a snapshot for each page in the template so Percy can diff them.

I think we can link to a specific page in the template by adding #page=<page-id> to the end of the url in each test.
For example, if we want to test the first page the url value would be examples/visual-tests/amp-story/amp-story-page-attachment-v2.html#page=cover

In the next test, the url for page 2 would be examples/visual-tests/amp-story/amp-story-page-attachment-v2.html#page=inline-custom-text

We will know it's working when new snapshots show up for review in the PR.

We also need to be sure the experiment is on.
It looks like that can be done in the html template by following the instructions in visual-tests:
https://github.com/ampproject/amphtml/blob/main/test/visual-diff/visual-tests#L75-L80

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I turned on the experiment! And I added a test for each page

@raxsha raxsha requested a review from processprocess May 7, 2021 01:24
processprocess
processprocess requested changes May 7, 2021
View reviewed changes
Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

I'm not sure that the visual tests are set up.
We will know they are set up properly when we see percy diffs for each page for review.
Please see the comment about having a unique ID for each page.

@raxsha raxsha requested a review from processprocess May 18, 2021 17:44
@amp-owners-bot
Copy link

amp-owners-bot bot commented May 18, 2021 •
edited
Loading

Hey @gmajoulet, @newmuis, @Enriqe! These files were changed:

extensions/amp-story/1.0/amp-story-open-page-attachment.css
extensions/amp-story/1.0/amp-story-page-attachment.css
extensions/amp-story/1.0/amp-story-page.js

@processprocess processprocess mentioned this pull request May 18, 2021
Closed
@processprocess
Copy link
Contributor

processprocess commented May 18, 2021 •
edited
Loading

Diffs look like they're all snapping which is awesome!

The font and vertical margin is looking strange in the diffs 🤔.
Screen Shot 2021-05-18 at 4 21 11 PM

When running locally it looks good. The margins are even around the circle on the left:
Screen Shot 2021-05-19 at 11 45 01 AM

@raxsha raxsha removed the request for review from gmajoulet May 18, 2021 21:51
raxsha and others added 5 commits May 19, 2021 12:31
@raxsha
adding important
5f6fab5
@processprocess
Improve targeting of img in remote-content.
f1cfe36
@processprocess
Refactor go to page
84c0e2f 
Removes tap simulation to navigate to page. Removes timeout. Improves selector to be sure page is visible.
@processprocess
Use same interactive tests for desktop.
06ada9e
@processprocess
Remove desktop.js
92793f2 
JS can be re-used for desktop and mobile.
processprocess
processprocess approved these changes May 24, 2021
View reviewed changes
Copy link
Contributor

@processprocess processprocess left a comment

Choose a reason for hiding this comment

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

Looks awesome. Thank you for setting these up!
I think these will help code cleanup be much easier.

A note that I found a way to simplify the JS for selecting the page and added it to the PR.
I think it's a pattern we can use in the future for multipage tests.

@processprocess processprocess merged commit 048f036 into ampproject:main May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@processprocess processprocess processprocess approved these changes

@estherkim estherkim estherkim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@raxsha @processprocess @estherkim