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.

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

Testing coverage for image #1063

Merged
merged 6 commits into from
Feb 13, 2019
Merged

Testing coverage for image #1063

merged 6 commits into from
Feb 13, 2019

Conversation

danielamorse
Copy link
Collaborator

@danielamorse danielamorse commented Jan 29, 2019 •
edited by sghoweri
Loading

Jira

Summary

Added developer and QA testing instructions for the Bolt Image component.

Details

  • Added TESTING.md file with developer and QA testing instructions.
  • Jest tests were already setup.
  • For VR testing, I suggest we use the View all page. It's lengthy but maybe still within range? All but the last section (Zoom) are needed for VR testing. Thoughts?

How to test

  • Review TESTING.md. Is it clear and accurate? Is there anything missing? Should anything be removed?
  • Review Jest tests. Any other tests needed at this point, or good for now?

remydenton
remydenton reviewed Jan 31, 2019
View reviewed changes
Copy link
Collaborator

@remydenton remydenton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good overall. I did add one inline comment asking when to go with the Cucumber format-- putting myself in QA's shoes, I think it would actually be easier for me to see concrete steps for testing something like lazyloading.

packages/components/bolt-image/TESTING.md
1. When I drag my finger around the tablet image
1. Then other zoomed-in parts of the tablet image are shown
1. When I remove my finger or drag it outside of the tablet image
1. Then the tablet image returns to its original state
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good choice having separate ones for mobile and desktop. In my review for button, I argued they should be condensed, but the behavior is so different here that I think the different versions make sense.

packages/components/bolt-image/TESTING.md Show resolved Hide resolved
remydenton
remydenton requested changes Feb 1, 2019
View reviewed changes
packages/components/bolt-image/TESTING.md Outdated
## Scenario: Image Zoom (desktop)

1. Given I am using a "desktop" browser
1. And I am viewing the [Image Zoom page](http://localhost:3000/pattern-lab/patterns/02-components-image-30-image-zoom-variation/02-components-image-30-image-zoom-variation.html)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just one last thing I noticed before this is good to go: I think it's good practice to replace localhost urls with https://master.boltdesignsystem.com (which you already did in the rest of this document, just not in the two image zoom tests).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@remydenton, definitely. Good catch. I replaced it in this commit.

remydenton
remydenton approved these changes Feb 5, 2019
View reviewed changes
Copy link
Collaborator

@remydenton remydenton left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @danielamorse!

sghoweri
sghoweri reviewed Feb 6, 2019
View reviewed changes
packages/components/bolt-image/TESTING.md
- Lazyloaded JPG images get special treatment. They transition from a blurred low-resolution placeholder image to a sharp lazyloaded image.
- Lazyloaded JPG images have the class `c-bolt-image__lazyload--blur`.
- All lazyloaded images are loaded after the window `onload` event, including not-in-view images.
- This setting is controlled by the `preloadAfterLoad` option in the `lazySizes` plugin. See [the docs](https://github.com/aFarkas/lazysizes#js-api---options).
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@sghoweri sghoweri mentioned this pull request Feb 6, 2019
Closed
@sghoweri
Copy link
Contributor

sghoweri commented Feb 6, 2019

@danielamorse @adamszalapski I could see two specific follow-up updates to build off of this:

  1. Using Nightwatch to check for lazyloading bevahior (based on scrolling + image visibility)
  2. Separately using Mocha to ensure the JS logic and class name behavior is working as expected

@danielamorse
Copy link
Collaborator Author

@danielamorse @adamszalapski I could see two specific follow-up updates to build off of this:

  1. Using Nightwatch to check for lazyloading bevahior (based on scrolling + image visibility)
  2. Separately using Mocha to ensure the JS logic and class name behavior is working as expected

Noted 👍 I can look into these two updates as part of the image web component conversion.

@sghoweri
Copy link
Contributor

@danielamorse can you try updating this PR branch with the latest up on master to see if the Travis build passes as expected?

@sghoweri sghoweri merged commit fd14df3 into master Feb 13, 2019
@sghoweri sghoweri deleted the feature/testing-coverage-for-image branch February 13, 2019 18:12
@sghoweri sghoweri mentioned this pull request Apr 16, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sghoweri sghoweri sghoweri approved these changes

@remydenton remydenton remydenton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@danielamorse @sghoweri @remydenton