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

DatePicker: Use compact button size #65653

Merged
merged 2 commits into from
Sep 25, 2024
Merged

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Sep 25, 2024

What?

This PR updates the DatePicker to use compact size for the Prev and Next buttons.

Why?

In #65018 we're replacing the size violations, but that applies only to components outside the @wordpress/components package. Here, we're updating the button sizes within a @wordpress/components to match the sizing guidelines.

How?

We're making the prev / next buttons compact.

My understanding is that icon buttons should be compact (32px) and not 40px (nor the 36px that they are now). Can you verify that's the case @WordPress/gutenberg-design?

Testing Instructions

  • Test the "Prev" and "Next" buttons of the DatePicker make sense and look/work well with the compact sizing:
    • In Storybook: /?path=/story/components-datepicker--default
    • In the post editor by selecting a publish date.

Testing Instructions for Keyboard

Same

Screenshots or screencast

Before After
Screenshot 2024-09-25 at 17 36 51 Screenshot 2024-09-25 at 17 34 11

@tyxla tyxla added [Type] Enhancement A suggestion for improvement. [Package] Components /packages/components labels Sep 25, 2024
@tyxla tyxla requested a review from a team September 25, 2024 14:44
@tyxla tyxla self-assigned this Sep 25, 2024
@tyxla tyxla requested a review from ajitbohra as a code owner September 25, 2024 14:44
Copy link

github-actions bot commented Sep 25, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Changes LGTM, although they don't really have an impact because of style overrides.

As we are exploring designs for expressing a date range, it could be a good opportunity to rethink this aspect too — I'm not really sure we need to use a Button here.

@tyxla
Copy link
Member Author

tyxla commented Sep 25, 2024

Changes LGTM, although they don't really have an impact because of style overrides.

I did spot the width styles, but they were making the button a square, which is why I didn't remove them. I guess if we want to remove them, we'll need to do it in a coordinated effort that tests all icon buttons across the editor. The height is actually considered.

As we are exploring designs for expressing a date range, it could be a good opportunity to rethink this aspect too — I'm not really sure we need to use a Button here.

Icon buttons are a thing, aren't they? Or do you mean we can use "Prev" and "Next" links instead of arrow buttons? Or perhaps in a border sense using the native datepicker UIs?

@ciampo
Copy link
Contributor

ciampo commented Sep 25, 2024

🤦 Ignore my previous comment — for some reason, I thought that we were talking about the button used for each day in the calendar, and not the one used for prev/next months.

Nonetheless, changes LGTM

@tyxla tyxla merged commit 0bfec0f into trunk Sep 25, 2024
62 checks passed
@tyxla tyxla deleted the update/datepicker-button-size branch September 25, 2024 15:44
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Sep 25, 2024
@mirka mirka added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Oct 1, 2024
@noisysocks noisysocks added [Type] Code Quality Issues or PRs that relate to code quality Backported to WP Core Pull request that has been successfully merged into WP Core and removed [Type] Enhancement A suggestion for improvement. Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Oct 6, 2024
Copy link

github-actions bot commented Oct 6, 2024

There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch.

PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk.

# Checkout the wp/6.7 branch instead of trunk.
git checkout wp/6.7
# Create a new branch for your PR.
git checkout -b my-branch
# Cherry-pick the commit.
git cherry-pick 0bfec0f91843b993a929e6aafe9548cf07690eea
# Check which files have conflicts.
git status
# Resolve the conflict...
# Add the resolved files to the staging area.
git status
git add .
git cherry-pick --continue
# Push the branch to the repository
git push origin my-branch
# Create a PR and set the base to the wp/6.7 branch.
# See https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/changing-the-base-branch-of-a-pull-request.

noisysocks pushed a commit that referenced this pull request Oct 6, 2024
* DatePicker: Use compact button size

* CHANGELOG

Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>
@tyxla
Copy link
Member Author

tyxla commented Oct 7, 2024

@noisysocks if I understand correctly, this has already been backported, but let me know if I can help with anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants