-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add padding support to element screenshot (#4440) #5078
Conversation
766a0f2
to
bad539d
Compare
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. |
bad539d
to
5e63453
Compare
This looks really good. I like the API and the implementation and tests look solid. @brian-mann What do you think about the API? |
There was a problem hiding this 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
40c1686
to
b5ec6d2
Compare
@sebinsua Ok, so I checked with @brian-mann and we are officially approving the API change that is being proposed with the 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. |
e2d9ea1
to
3aa29af
Compare
Do you want them as part of this PR? Where are the schemas, docs and type definitions defined? |
3aa29af
to
52d4b82
Compare
The type definitions can be part of this PR. Augment cli/types/index.d.ts with the new 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 |
f2ccb00
to
270770b
Compare
@jennifer-shehane @brian-mann @chrisbreiding thanks a lot for your help thus far. I believe this is now ready for another review 🙂 |
270770b
to
f2eb2ff
Compare
0fc2324
to
b6d07c8
Compare
@jennifer-shehane @chrisbreiding We've updated the PR with types and changes related to your comments. (/cc @NMinhNguyen) |
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>
b6d07c8
to
a93260d
Compare
@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. |
@sebinsua Yes, we have your PR on our radar for review. |
@sebinsua I requested a couple changes on the docs PR. Once those are addressed, this should be good to go. |
Changes have been addressed and re-reviewed
Sorry, we are introducing a few things to our builds that are causing some hiccups, specifically #5312 😢 We'll get it in though. |
* 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
Pre-merge Tasks
cypress-documentation
been submitted to document any user-facing changes? Add padding support to element screenshot cypress-documentation#2139Why?
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
orrunner
.We tried two different approaches:
elPosition
values to account for padding.They are both committed.
We're leaning towards the second approach, because it doesn't mutate the DOM.