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.

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

Add padding support to element screenshot (#4440) #5078

Conversation

sebinsua
Copy link
Contributor

@sebinsua sebinsua commented Sep 3, 2019

Pre-merge Tasks

Why?

Screenshotting an element will often crop out edges of the element (e.g. box-shadow might not be visible). It should be possible to tell Cypress to capture a padding around an element.

Approach

This implementation only adds padding support to element screenshots, since we did not feel it makes sense to modify screenshots of the fullPage, viewport or runner.

We tried two different approaches:

  1. We tried to create an extra element with adjusted dimensions from the original element.
  2. We tried to adjust the elPosition values to account for padding.

They are both committed.

We're leaning towards the second approach, because it doesn't mutate the DOM.

@CLAassistant
Copy link

CLAassistant commented Sep 3, 2019

CLA assistant check
All committers have signed the CLA.

@sebinsua sebinsua force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch 2 times, most recently from 766a0f2 to bad539d Compare September 4, 2019 10:47
@sebinsua
Copy link
Contributor Author

sebinsua commented Sep 4, 2019

@chrisbreiding @brian-mann

I noticed that you've both worked on the screenshot feature before. I've attempted an implementation that allows users to add padding to element screenshots and would like to get your opinion on: potential issues, API design, etc.

@jennifer-shehane jennifer-shehane requested a review from a team September 20, 2019 19:30
@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from bad539d to 5e63453 Compare September 22, 2019 04:25
@chrisbreiding
Copy link
Contributor

This looks really good. I like the API and the implementation and tests look solid.

@brian-mann What do you think about the API?

Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

This is awesome work. Left a couple of comments.

Does this also address the issue of screenshotting elements with 0 width/height by chance ? #5149

packages/driver/src/cy/commands/screenshot.coffee Outdated Show resolved Hide resolved
packages/driver/src/cypress/error_messages.coffee Outdated Show resolved Hide resolved
@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from 40c1686 to b5ec6d2 Compare September 25, 2019 00:28
packages/driver/src/cy/commands/screenshot.coffee Outdated Show resolved Hide resolved
packages/driver/src/cypress/screenshot.coffee Outdated Show resolved Hide resolved
packages/driver/src/cypress/screenshot.coffee Outdated Show resolved Hide resolved
packages/driver/src/cypress/screenshot.coffee Outdated Show resolved Hide resolved
@jennifer-shehane
Copy link
Member

@sebinsua Ok, so I checked with @brian-mann and we are officially approving the API change that is being proposed with the padding option. So, feel free to move forward with the schemas, docs, and type definitions needed.

There are some comments for updates to the code and further review wanted to the code that we'll continue.

Again, thanks for the contribution - just need to clean up some things based off of our code conventions.

@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch 6 times, most recently from e2d9ea1 to 3aa29af Compare September 26, 2019 02:52
@sebinsua
Copy link
Contributor Author

@sebinsua Ok, so I checked with @brian-mann and we are officially approving the API change that is being proposed with the padding option. So, feel free to move forward with the schemas, docs, and type definitions needed.

Do you want them as part of this PR? Where are the schemas, docs and type definitions defined?

@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from 3aa29af to 52d4b82 Compare September 26, 2019 11:03
@chrisbreiding
Copy link
Contributor

@sebinsua Ok, so I checked with @brian-mann and we are officially approving the API change that is being proposed with the padding option. So, feel free to move forward with the schemas, docs, and type definitions needed.

Do you want them as part of this PR? Where are the schemas, docs and type definitions defined?

The type definitions can be part of this PR. Augment cli/types/index.d.ts with the new padding option. There should be no need for schema changes; that's only needed for config (cypress.json) changes.

For documentation, that's a separate repo, so it will need to be a separate PR: https://github.com/cypress-io/cypress-documentation/blob/develop/source/api/commands/screenshot.md

@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from f2ccb00 to 270770b Compare September 27, 2019 00:13
@NMinhNguyen
Copy link

@jennifer-shehane @brian-mann @chrisbreiding thanks a lot for your help thus far. I believe this is now ready for another review 🙂

@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from 270770b to f2eb2ff Compare September 27, 2019 09:21
@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch 3 times, most recently from 0fc2324 to b6d07c8 Compare September 27, 2019 09:57
@sebinsua
Copy link
Contributor Author

@jennifer-shehane @chrisbreiding We've updated the PR with types and changes related to your comments. (/cc @NMinhNguyen)

sebinsua and others added 2 commits September 30, 2019 13:14
Closes cypress-io#5149

Co-authored-by: Minh Nguyen <minhnguyenxx@gmail.com>
Co-authored-by: Jennifer Shehane <shehane.jennifer@gmail.com>
Closes cypress-io#4440

Co-authored-by: Minh Nguyen <minhnguyenxx@gmail.com>
Co-authored-by: Jennifer Shehane <shehane.jennifer@gmail.com>
@NMinhNguyen NMinhNguyen force-pushed the feat/add-padding-support-to-element-screenshot-4440 branch from b6d07c8 to a93260d Compare September 30, 2019 12:15
sebinsua added a commit to sebinsua/cypress-documentation that referenced this pull request Oct 7, 2019
@sebinsua
Copy link
Contributor Author

sebinsua commented Oct 8, 2019

@jennifer-shehane Will anybody get time to review the code we wrote in response to your comments this week? Also, I've put up a PR for some docs here.

@jennifer-shehane
Copy link
Member

@sebinsua Yes, we have your PR on our radar for review.

@chrisbreiding
Copy link
Contributor

@sebinsua I requested a couple changes on the docs PR. Once those are addressed, this should be good to go.

sebinsua added a commit to sebinsua/cypress-documentation that referenced this pull request Oct 9, 2019
@jennifer-shehane jennifer-shehane dismissed brian-mann’s stale review October 9, 2019 19:12

Changes have been addressed and re-reviewed

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Oct 9, 2019

Sorry, we are introducing a few things to our builds that are causing some hiccups, specifically #5312 😢 We'll get it in though.

@jennifer-shehane jennifer-shehane merged commit 116a634 into cypress-io:develop Oct 11, 2019
jennifer-shehane pushed a commit to cypress-io/cypress-documentation that referenced this pull request Oct 11, 2019
* Develop (#2059)

* add instrument-cra plugin link

* add another cy.within example

* Fix typo

* lowercase 'TR'


Co-authored-by: Jennifer Shehane <jennifer@cypress.io>

* Update Continuous Integration doc with details for AWS Amplify… (#2124)

* change 'default org' to 'personal org' (#2133)

* add redirect (#2137)

* Update cypress open --browser section (#2134)



Co-authored-by: Kevin Old <kevin@cypress.io>

* Add note about potential IDE TS server issues (#2136)

* add note about potential IDE TS server issues

* include js in file types that support restart command

* Add padding support to element screenshot

See: cypress-io/cypress#5078
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants