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

[Synthetics] Fix mobile synthetics image clipping #106128

Merged

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Jul 19, 2021

Summary

Resolves elastic/uptime#339.

The default sizes we use for images are hardcoded. They work well for landscape-shaped images; portrait-shaped images like the ones produced by mobile emulation are getting clipped by the hardcoded value.

We can limit the width to a max-size and allow height to match. The plus side of this solution is it's very simple, and it works for both image sizes. The drawback is it will reduce the size of the image we show for desktop-style images, but it will not clip anything. Users will also still have the ability to expand the screenshot to see it in the full-screen viewer mode.

This is a minimum-viable fix. I am open to suggestions on how to make this better, especially if it means the existing experience won't change.

Checklist

Delete any items that are not applicable to this PR.

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk Probability Severity Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space. Low High Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. High Low Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled. Medium High Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability polish v7.14.0 v7.15.0 labels Jul 19, 2021
@justinkambic justinkambic self-assigned this Jul 19, 2021
@justinkambic justinkambic requested a review from a team as a code owner July 19, 2021 16:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic force-pushed the 339/fix-mobile-image-clipping branch from 805e443 to d2eea2b Compare July 19, 2021 17:05
@justinkambic
Copy link
Contributor Author

I tested this with full screenshot-style monitors as well (as opposed the blocking optimization we introduced for 1.14). Seemed to work fine.
image

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
uptime 946.5KB 947.1KB +571.0B

History

  • 💔 Build #139469 failed bb259667ff3c22e63cef618177cf57cf0f17d3a2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @justinkambic

@justinkambic justinkambic added the auto-backport Deprecated - use backport:version if exact versions are needed label Jul 19, 2021
@paulb-elastic
Copy link
Contributor

Checked this with local build of the branch and it works as expected for the iphone viewport I tried originally:
Video_2021-07-20_103622

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

LGTM. I'm wondering, though, if we wanted to do anything to reduce the height of the rows, as they are quite large for device emulation.
Screen Shot 2021-07-20 at 10 11 08 AM

@justinkambic
Copy link
Contributor Author

justinkambic commented Jul 20, 2021

I'm wondering, though, if we wanted to do anything to reduce the height of the rows, as they are quite large for device emulation.

@dominiqueclarke I am ++ for doing this. I'll create an enhancement request.

EDIT:

Issue here: elastic/uptime#344

@justinkambic justinkambic merged commit ff84ac9 into elastic:master Jul 20, 2021
@justinkambic justinkambic deleted the 339/fix-mobile-image-clipping branch July 20, 2021 16:06
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 20, 2021
* Change hardcoded image size value to prevent clipping of mobile synthetics screenshots.

* Compute max values for `ref`-style screenshots to improve display ux.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 20, 2021
* Change hardcoded image size value to prevent clipping of mobile synthetics screenshots.

* Compute max values for `ref`-style screenshots to improve display ux.
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.14
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jul 20, 2021
* Change hardcoded image size value to prevent clipping of mobile synthetics screenshots.

* Compute max values for `ref`-style screenshots to improve display ux.

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
kibanamachine added a commit that referenced this pull request Jul 20, 2021
* Change hardcoded image size value to prevent clipping of mobile synthetics screenshots.

* Compute max values for `ref`-style screenshots to improve display ux.

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience polish release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 v7.15.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uptime not handling different sized screenshots (mobile) particularly well
5 participants