-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Automattic for Agencies: Redesign hosting marketplace page #90837
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~1478 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
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.
d1033e0
to
d7e3640
Compare
Thanks for the review, @keoshi!
Yes, I added an Screen.Recording.2024-05-17.at.2.51.50.PM.mov |
I also noticed that the border-radius on the new design uses 8px. @keoshi is this correct? |
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.
Thanks for working on this. Works great. I left few comments though.
Thanks for the review, @jkguidaven! I have addressed your comments: 27fb2ce |
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.
Code wise this LGTM. Changes also works great as per test instructions. 👍
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.
Looks great to me, Yashwin! Have two small comments that are non-blocking, so I'm approving the PR.
Doesn't respond to simple tap
On mobile, it seems like the tooltip is only shown when tap + moving the tap point is involved. In other others, it doesn't seem to respond to a simple tap on the same spot.
I'm testing this in the laptop, simulating a touch device, so it might be that causing the issue. If we can't fix it on mobile, it might be worth to always display the tooltip on touch devices.
Shared benefit should be full-width
According to the latest designs by @jeffgolenski — https://www.figma.com/design/sX0uNGAgDUceP3nKu7iQ4z/Purchase-flows?node-id=8346-138635&t=X10Un6R2s1Trcn6F-11 — the bottom section that groups shared benefits should be full-width.
Also notice the title should be "Included with Standard & Premier hosting".
Thanks for all your hard work on this, @yashwin !
9c834bd
to
d064632
Compare
d064632
to
34f430d
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Thanks for the review, @keoshi!
I think this should work on mobile devices. I'll do a round of testing on mobile once this PR is shipped and fix it if needed in another PR.
Thanks for linking the latest design.
I have updated it now, including some text changes: 34f430d |
Perrrfect — ship it! 🚢 |
Can we pause on shipping this one @yashwin please, sorry if it's a pain. |
Want to get the final go-ahead from leadership before we do. |
Thanks for reverting 👍 seeking clarity from leadership now. I'll let you know as soon as I do. |
@simonktnga8c: Sure, I have shipped the revert PR and created a new one now: #90890 |
Resolves https://github.com/Automattic/jetpack-roadmap/issues/1563
Proposed Changes
This PR redesigns the hosting marketplace page
Testing Instructions
Pre-merge Checklist