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

Performance Profiler: Add preview box to timeline screenshots #95515

Open
wants to merge 4 commits into
base: trunk
Choose a base branch
from

Conversation

epeicher
Copy link
Contributor

@epeicher epeicher commented Oct 18, 2024

Resolves https://github.com/Automattic/dotcom-forge/issues/9192

Proposed Changes

  • Displays a modal when clicking on a Screenshot of the timeline preview
Logged-in Logged-out
CleanShot 2024-10-18 at 14 06 10@2x CleanShot 2024-10-18 at 14 06 30@2x

Why are these changes being made?

  • For consistency with a similar behaviour in the tool. To improve the UX by allowing the user to preview the images in a bigger size.

Testing Instructions

  • Apply this changes and navigate to /speed-test tool with a valid URL
  • Go to the timeline and click on an image
  • Check a modal is displayed with the contents of the clicked image
  • Repeat using the logged-in version (activate the flag performance-profiler/logged-in)
  • Check a modal is displayed with the contents of the clicked image

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

Copy link

github-actions bot commented Oct 18, 2024

Link to live branch is being generated...
Please wait a few minutes and refresh this page.

@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug add/image-preview-to-timeline on your sandbox.

@matticbot
Copy link
Contributor

matticbot commented Oct 18, 2024

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~255 bytes added 📈 [gzipped])

name                  parsed_size           gzip_size
site-performance           +926 B  (+0.1%)     +255 B  (+0.1%)
performance-profiler       +926 B  (+0.3%)     +255 B  (+0.2%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Collaborator

@WBerredo WBerredo left a comment

Choose a reason for hiding this comment

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

I can see some issues that we might want to dive in:

  • We might want to add a background to the X button for cases where it mixes with the background. Something like a black background with some opacity.
  • On the logged in version the modal is centralized on the screen but I think it should be centralized on the Performance Profiler section, so bringing the sidebar width into the calculation might help.
  • The resolution is compromised we see it on the modal (worse on bigger screens), so if we don't receive a different source for the images, it might help to add some image treatment to amplify it without losing (much) readability.

CleanShot 2024-10-18 at 11 07 15@2x

@epeicher
Copy link
Contributor Author

I can see some issues that we might want to dive in:

  • We might want to add a background to the X button for cases where it mixes with the background. Something like a black background with some opacity.
  • On the logged in version the modal is centralized on the screen but I think it should be centralized on the Performance Profiler section, so bringing the sidebar width into the calculation might help.
  • The resolution is compromised we see it on the modal (worse on bigger screens), so if we don't receive a different source for the images, it might help to add some image treatment to amplify it without losing (much) readability.

CleanShot 2024-10-18 at 11 07 15@2x

Thanks for your comments @WBerredo, I would be interested in knowing if all comments are a blocker for this task or if any of them can be accomplished with a follow-up task.

This is my view:

We might want to add a background to the X button for cases where it mixes with the background. Something like a black background with some opacity.

That's a good catch. This can be simple to add as part of this task. I will investigate options. I'm pinging @matt-west for any tips.

On the logged in version the modal is centralized on the screen but I think it should be centralized on the Performance Profiler section, so bringing the sidebar width into the calculation might help.

This is the same approach used for the screenshots displayed in the Insights section. They were added as part of this PR #94582 and updated as part of this other PR #94890. In my opinion, because this is a Modal, centering with regards to the right screen could look misaligned. I will also loop in @matt-west for another opinion.

The resolution is compromised we see it on the modal (worse on bigger screens), so if we don't receive a different source for the images, it might help to add some image treatment to amplify it without losing (much) readability.

I am not sure if the extra CPU work required for this is worthy for a potentially minimally used feature here. But I would create a follow-up for further investigation/discussion. What do you think?

@WBerredo
Copy link
Collaborator

You are right, sorry for not making clear what is a blocker or not.

That's a good catch. This can be simple to add as part of this task.

Agreed, I wouldn't merge without this change but it should be very easy to accomplish too.

This is the same approach used for the screenshots displayed in the Insights section. They were added as part of this PR #94582 and updated as part of this other PR #94890. In my opinion, because this is a Modal, centering with regards to the right screen could look misaligned. I will also loop in @matt-west for another opinion.

Yeah, I'm not very sure about this topic either, and Matt is definitely a better person to define this :)

I am not sure if the extra CPU work required for this is worthy for a potentially minimally used feature here. But I would create a follow-up for further investigation/discussion. What do you think?

I agree it can be a follow-up task but I would also defer to @matt-west the decision if its worth to pursue or not.


All in all, I would just use the first one as a blocker.

@epeicher
Copy link
Contributor Author

epeicher commented Oct 18, 2024

@WBerredo I have tried different solutions, like just increasing the contrast in the filter property, but I have found scenarios where it does not look great, for example, when there is a cart icon behind. Eventually, I have implemented a solution that adds a two different icon backgrounds depending on the image background.

Please find an explanation from claude here Here's how this CSS-only solution works:

We create two pseudo-elements (::before and ::after) that will serve as our adaptive background.
The ::before element has a white background with mix-blend-mode: difference. This inverts dark backgrounds but leaves light backgrounds mostly unchanged.
The ::after element has a black background with mix-blend-mode: screen. This lightens dark backgrounds but leaves light backgrounds mostly unchanged.
The combination of these two effects creates a background that adapts to be visible on both light and dark underlying images or colors.
The SVG icon itself uses mix-blend-mode: difference to ensure it remains visible against the adaptive background.

These are the results in different white and black images with carts or other icons:

Example 1 Example 2 Example 3 Example 4
CleanShot 2024-10-18 at 17 41 27@2x CleanShot 2024-10-18 at 17 41 56@2x CleanShot 2024-10-18 at 17 41 36@2x CleanShot 2024-10-18 at 17 41 49@2x

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 18, 2024
Copy link
Collaborator

@WBerredo WBerredo left a comment

Choose a reason for hiding this comment

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

Eventually, I have implemented a solution that adds a two different icon backgrounds depending on the image background.

🤯 This is even better than I anticipated. TIL and IMHO it deserves a P2.

LGTM! 🚢

PS: I added a cursor pointer to the thumbnail as I have forgot to mention the first time.

@matt-west
Copy link
Contributor

Sorry for the slow response.

In my opinion, because this is a Modal, centering with regards to the right screen could look misaligned

Agreed, the modal should be centred based on the full-width of the window.

The resolution is compromised we see it on the modal (worse on bigger screens), so if we don't receive a different source for the images, it might help to add some image treatment to amplify it without losing (much) readability.

If we can’t get better resolution images from Google, I don’t think we should try to upscale them on our end. PageSpeed Insights doesn’t allow you to expand the images in their timeline. While this would be a nice feature to have, I don’t think we should add it if the screenshots will always look this pixelated. It doesn’t add any value beyond how they are currently displayed in the timeline.

Eventually, I have implemented a solution that adds a two different icon backgrounds depending on the image background.

@epeicher This is a really cool solution! Nice work. Can you please use the closeSmall icon instead so that there’s some whitespace around the X.

@epeicher
Copy link
Contributor Author

epeicher commented Oct 21, 2024

Hi @matt-west , I have been having a look at the Modal component, docs here, and I have not found a way to override the close icon. Also, looking at the source code, it seems the icon used is close from @wordpress/icons and it does not allow to configure it.

We can always extend the Modal component for our use-case, but alternatively, if I reduce the width of the icon with a single CSS property, we've got the extra padding.

Are you happy with this approach?

Before After
CleanShot 2024-10-21 at 12 46 25@2x CleanShot 2024-10-21 at 12 38 26@2x

@matt-west
Copy link
Contributor

Yep, that works. Thanks @epeicher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants