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

Add gray background to filter buttons in design picker #97168

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

madhusudhand
Copy link
Member

Related to https://github.com/Automattic/dotcom-forge/issues/10014

Proposed Changes

  • It adds gray background to the filter buttons in design picker to match the designs.

Before
image

After
image

Why are these changes being made?

  • To match the design spec
image

Testing Instructions

  • Verify the button UI in design picker

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)?

@madhusudhand madhusudhand requested a review from a team December 6, 2024 10:10
@madhusudhand madhusudhand self-assigned this Dec 6, 2024
@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 Dec 6, 2024
@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.

@madhusudhand
Copy link
Member Author

❓ Designs suggest a less gap between the buttons. Do you prefer to override the gap in design-picker only or in the base component?
Note: the component is being used in multiple places. And most of them have page specific overrides which makes the component appear differently.

@taipeicoder
Copy link
Contributor

@madhusudhand will there be a follow-up PR to make the filters UI match the design spec? They still look very different.

@taipeicoder
Copy link
Contributor

❓ Designs suggest a less gap between the buttons. Do you prefer to override the gap in design-picker only or in the base component? Note: the component is being used in multiple places. And most of them have page specific overrides which makes the component appear differently.

Yes, overrides in the design picker only.

@taipeicoder taipeicoder removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 10, 2024
Copy link
Contributor

@arthur791004 arthur791004 left a comment

Choose a reason for hiding this comment

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

Do we want to update the following UI as well?

  • The background color of the selected item
  • The background color under the "More" dropdown
    image

Not related to this PR, but I found it looks weird on smaller screen

image

Copy link
Contributor

@arthur791004 arthur791004 left a comment

Choose a reason for hiding this comment

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

We can address the above issue in the follow-up PRs 🙂

@taipeicoder
Copy link
Contributor

taipeicoder commented Dec 11, 2024

I've confirmed with @fditrapani in that we intend to override the styles for the filters in the Design Picker. Filippo also pointed out that https://wordpress.com/patterns also shares similar styling.

Creating the issue https://github.com/Automattic/dotcom-forge/issues/10108 to track this work.

@madhusudhand
Copy link
Member Author

@madhusudhand will there be a follow-up PR to make the filters UI match the design spec? They still look very different.

I will address all the UI issues in the followup PR. Merging this for now.

@madhusudhand madhusudhand merged commit 24ee683 into trunk Dec 11, 2024
13 checks passed
@madhusudhand madhusudhand deleted the design-picker-filter-button-bg branch December 11, 2024 07:19
@fditrapani
Copy link
Contributor

Not related to this PR, but I found it looks weird on smaller screen

Thanks for pointing that out @arthur791004. We should make some adjustments on smaller screens for sure. I've shared a mobile design which displays the filters on two rows and has horizontal scrolling (without the more button). Let me know if that's enough or if you need me to mock anything else up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants