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

Change the week header and left button style to meet the date spacing #27730

Merged
merged 3 commits into from
Jan 7, 2021

Conversation

hsingyuc
Copy link
Contributor

Description

Fix #27536
Change the week header and left button style to meet the date spacing.

How has this been tested?

Tested in English and in Arabic (rtl), both render as expected

Screenshots

  1. Before

before_change

1. After

change_spacing

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Dec 15, 2020
@gziolo gziolo added Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended and removed First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository labels Jan 5, 2021
@gziolo gziolo requested a review from jasmussen January 5, 2021 07:22
Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

This is awesome! Small, focused PR! Thank you. Before:

before

After:

after

The month navigation buttons have better alignment.

I'm seeing an issue in arabic, though:

Screenshot 2021-01-05 at 08 52 00

The button to navigate left takes up the whole screen. This may have been a pre-existing issue, but because this PR aims to close the ticket that reported the issue, it seems worth fixing.

From a little debugging in the inspector, I found this rule:

.components-datetime__date .DayPickerNavigation_leftButton__horizontalDefault {
    right: 13px;
}

If that gets changed to:

.components-datetime__date .DayPickerNavigation_leftButton__horizontalDefault {
    left: 13px;
}

all is fine.

In other words, it appears that the left and right positioning gets auto-rewritten for RTL, when it shouldn't be. So in the code where it says left, you can add the following line before it, to have it not be rewritten: /*rtl:ignore*/.

If we get that in, we can ship this. Thanks again.

@hsingyuc
Copy link
Contributor Author

hsingyuc commented Jan 5, 2021

@jasmussen Thanks for the tip! The change has been pushed.

@jasmussen
Copy link
Contributor

LTR is good:

ltr

But I'm still seeing an issue with RTL:

rtl

It looks like you need to not only be explicit about the left value, but the right value as well.

This might work:

		/*!rtl:begin:ignore*/
		left: 13px;
		right: auto;
		/*!rtl:end:ignore*/

Thanks for your patience!

@hsingyuc
Copy link
Contributor Author

hsingyuc commented Jan 6, 2021

@jasmussen Thank you! The begin/end ignore is a good addition! I've pushed that change. Could you confirm if you have pulled the latest changes and built the scripts? I see the arrows properly aligned in RTL without the need for right: auto;. Thank you for your time!

@jasmussen
Copy link
Contributor

Thanks again. I'm about to head out for the evening but promise I will test properly tomorrow morning my time, and approve if it works as I think it will. But in the mean time, someone else should feel free to test and approve.

@hsingyuc
Copy link
Contributor Author

hsingyuc commented Jan 6, 2021

It looks like this PR may also resolve #27535.

Also, I ran into another problem with the RTL calendar. The current month appears hidden because of the positioning of months in RTL mode.

螢幕快照 2021-01-06 下午3 18 46

Do you see this issue?

I think there's a quick fix for this by absolutely positioning the third calendar month in the container since we don't use calendar animations in GB, but this problem may be upstream in react dates. react-dates/react-dates#1552.

Copy link
Contributor

@jasmussen jasmussen left a comment

Choose a reason for hiding this comment

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

Thanks so much for this PR, and thanks for the patience. With the latest changes, this is solid in LTR:

Screenshot 2021-01-07 at 09 10 04

And RTL:

Screenshot 2021-01-07 at 09 10 31

Here's RTL before, for comparison:

Screenshot 2021-01-07 at 09 24 58

That arabic calendar does look fairly compressed to me, though, so there may be a separeate bug at play, but since that existed before this PR it's unrelated. Admittedly my Arabic is a little rusty, so I'll defer to speakers of that language.

Ship this, and thanks again!

@hsingyuc
Copy link
Contributor Author

hsingyuc commented Jan 7, 2021

The pleasure is all mine 🤝

@jasmussen jasmussen merged commit c3ff86d into WordPress:master Jan 7, 2021
@github-actions github-actions bot added this to the Gutenberg 9.8 milestone Jan 7, 2021
@hsingyuc hsingyuc deleted the fix/27536 branch January 7, 2021 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect Calendar Month Grid position
3 participants