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

Fix/design picker layout #98285

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

Fix/design picker layout #98285

wants to merge 7 commits into from

Conversation

fditrapani
Copy link
Contributor

Related to # #98176, #98170, #98283

Proposed Changes

This PR introduces a number of updates:

  • Cleans up and improves the spacing between sections
  • Improves the Theme thumbnail quality (they're currently blurry)
  • Updates the title alignment of the design picker to match the alignment for all the other screens
  • Fixes an issue on mobile where the filters appear cut off
  • Fixes an overlapping issue for Big Sky on mobile (in this case, I've removed it temporarily for mobile and will assess a better way to promote big sky on mobile)
  • Some quality improvements to rollovers and thumbnail details

Before: Desktop

image image

After: Desktop

image image

Before: Mobile

image

After: Mobile

image

Testing Instructions

Go through our onboarding until you reach the design picker step. Compare it against production. If possible, try upgrade to Premium before viewing the design picker to see the Big sky updates. You can use the following URL after your site is updates (just replace the URL):

/setup/site-setup/designSetup?siteSlug=aaa4315.wordpress.com&categories=blog%2Cnewsletter%2Cbusiness%2Clink-in-bio

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 UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
  • 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)?

@fditrapani fditrapani self-assigned this Jan 13, 2025
@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@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 Jan 13, 2025
@rcrdortiz
Copy link
Contributor

On some resolutions, the filter pane isn't centered correctly. There is more space on the right side than on the left side.
Screenshot 2025-01-13 at 18 08 45

@fditrapani
Copy link
Contributor Author

On some resolutions, the filter pane isn't centered correctly. There is more space on the right side than on the left side.

Thanks for catching that @rcrdortiz! There was a dynamic element to this section that was making it tricky to get it right. I just took another look and have an idea of how to handle it. I'll share update after I've gotten a chance to get a fix in!

Copy link
Contributor

@paulopmt1 paulopmt1 left a comment

Choose a reason for hiding this comment

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

I tested and validated:

  • We have a better theme thumbnail quality
  • Fixed mobile issue with Big Sky CTA
  • We're aligning everything in the center for certain screen sizes (>960px width): I didn't find any Figma here W9xI27S6Swvw5Ku21EbZvn-fi-9134_65080 with this new layout, so I may be missing some context, but I thought we wanted everything left-aligned.
  • With less than 960px width we have everything left-aligned again. Is that intentional?

image

  • Are these three different spacing intentional?
    image

  • Can we improve this vertical alignment?
    image

  • I liked the new hover effect on the theme thumbnails!

Screen.Recording.2025-01-13.at.16.32.54.mov

@fditrapani
Copy link
Contributor Author

Thanks for taking a look @paulopmt1!

We're aligning everything in the center for certain screen sizes (>960px width): I didn't find any Figma here W9xI27S6Swvw5Ku21EbZvn-fi-9134_65080 with this new layout, so I may be missing some context, but I thought we wanted everything left-aligned.

I initially shared a post about it here: p9Jlb4-fnF-p2. We've continued to have conversations about it. While I was in here working on this, I saw that it was easy enough to do on my own :)

With less than 960px width we have everything left-aligned again. Is that intentional?

Thanks for bringing this up. This is intentional since we don't have enough space to meaningfully show the options under topic. As I mentioned in another comment, there's a dynamic element here to contend with. I had a thought on how to handle it there, I'll see if it will work for this breakpoint too!

Are these three different spacing intentional?

Yes these are intentionally spaced this way. The two filters are grouped together visually while the button stands separately since it's a different action.

Can we improve this vertical alignment?

Good catch! Yes, we can improve this alignment.

I liked the new hover effect on the theme thumbnails!

Thank you! I forgot to mention it but I received feedback from multiple sources that our current hover effect looked like it was disabling the theme.

Copy link
Contributor

@paulopmt1 paulopmt1 left a comment

Choose a reason for hiding this comment

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

Once we fix that vertical alignment we're ready to ship. Great improvements!

@matticbot
Copy link
Contributor

matticbot commented Jan 14, 2025

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 fix/design-picker-layout on your sandbox.

@fditrapani
Copy link
Contributor Author

fditrapani commented Jan 14, 2025

@paulopmt1 I got the button alignment fixed:
image

Centre aligning the filters turned out to be trickier (cc @rcrdortiz). I was able to tighten the container with CSS so it visually appears centred for most use cases. It's still slightly off due to the dynamic way we're determining what's in the list and what's in the drop down but it's not as noticeable anymore:

image

The exception is if the community filter is preselected (Pick the collect donations goal):

image

The string is too long. To fix that, we can edit the label down to be "Community" only. cc @alaczek in case you see any problem with that.

I spoke to @taipeicoder about it and he said the JS code is very complicated and hard to work with so it might be best to stick with this CSS solution for now. I'm open to suggestions if anyone knows of a better way to handle it.

Copy link
Contributor

@rcrdortiz rcrdortiz left a comment

Choose a reason for hiding this comment

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

Looks good to me :shipit:

@taipeicoder
Copy link
Contributor

I spoke to @taipeicoder about it and he said the JS code is very complicated and hard to work with so it might be best to stick with this CSS solution for now. I'm open to suggestions if anyone knows of a better way to handle it.

Yes that component is unfortunately very capricious in the way it calculates when it should collapse items into the More menu 😕

@nuriapenya
Copy link

@fditrapani the gap between title + subtitle should be smaller, right?

screen 2025-01-14 at 12 03 13

Design mockups is 8px:

screen 2025-01-14 at 12 08 39

@fditrapani
Copy link
Contributor Author

the gap between title + subtitle should be smaller, right?

@nuriapenya. Good catch Nuria! I thought we were using the standard component so I didn't take notice but it appears we're not. I've fixed it up for now to match the other screens. We can circle back and standardize the components in a separate pass:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE [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.

6 participants