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

Polish datetime popover. #18235

Merged
merged 2 commits into from Nov 18, 2019
Merged

Polish datetime popover. #18235

merged 2 commits into from Nov 18, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Nov 1, 2019

The popover inherited a min-width of 260, which is insufficient, then overrode it just for the edit-post-schedule flow. This means the datetime component looks fine in the sidebar, but no-where else.

This PR changes to use a padding in the component itself, making it work everywhere.

Example of component used outside of sidebar, before:

Screenshot 2019-11-01 at 12 23 11

After:

Screenshot 2019-11-01 at 12 28 45

Copy link
Member

jorgefilipecosta left a comment

Hi @jasmussen, on the publish sidebar where the component is not used in a popover, there is a small visual change.
Before:
Screenshot 2019-11-01 at 13 07 55
After:
Screenshot 2019-11-01 at 13 06 42

I just wanted to confirm if that change is ok, or if we should add a rule to avoid that change.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 1, 2019

No, good catch, let me see if I can fix that one.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 1, 2019

Fixed, thanks.

@@ -5,6 +5,8 @@
/*rtl:end:ignore*/

.components-datetime {
padding: $grid-size;

This comment has been minimized.

Copy link
@youknowriad

youknowriad Nov 14, 2019

Contributor

Is this needed for all datetime components even outside the modal context? I feel this is specific to the editor usage.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 15, 2019

Author Contributor

You are right. This component that is included is horrendously messy with arbitrary inline styles. It needs a fresh approach, so I've committed some work in progress and will try a separate PR that goes at it differently.

Screenshot 2019-11-15 at 09 37 41

Screenshot 2019-11-15 at 09 37 35

┻━┻ ︵ヽ(`Д´)ノ︵ ┻━┻o

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 15, 2019

Author Contributor

But it is a good question, though, should it come with a padding or not?

The original version of this PR suggested that yes, this component should be born with padding, and if you need the component inline, as in the post-publish, you should override it there. Because otherwise you use the vanilla Popover component with the vanilla DateTime component, and you get this:

Screenshot 2019-11-15 at 09 46 50

I can remove some negative margins to fix the horizontal scrollbar:

Screenshot 2019-11-15 at 09 49 48

But the question becomes, if the datetime component should not have padding, should the popover?

I'm going to try something different, but this is context for why I added the padding.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Nov 15, 2019

Author Contributor

What's causing the horizontal scrollbar in the first place is this:

Screenshot 2019-11-15 at 09 58 34

And it is virtually impossible to refactor that width away because the entire datetime component referenced is reliant on this.

@jasmussen jasmussen force-pushed the fix/date-component branch 2 times, most recently from 1ea11a7 to 36a0ee7 Nov 15, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 15, 2019

Thank you, Riad, for the review in #18235 (review).

As you can see from the ensuing discussion I tried my best to work outside the baked in padding. However I failed to do so, and I see no way of fixing this issue without throwing out the calendar 3rd party library entirely and picking a different one. It's just not that good and has so many hardcoded inline styles that are necessary for it to function.

The good news is, I've applied a bunch of polish to this PR:

datetime

Same-height inputs, more cohesive padding and spacing. It looks way better, and it fixes the issue.

The only price is, when you mean to use the datetime component inline, as in the postpublish dialog, you have to unset the left and right paddings.

I think that's a small price to pay to move forward, without a total refactor. Let me know what you think. I've rebased and squashed this nicely so it should be easy to re-review.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 15, 2019

To be fair, I still don't get it. Why instead of overriding the padding for non-popover usage, do the opposite. No padding built-in and override the styles for the popover case only (which is the special case for me)

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 15, 2019

To be fair, I still don't get it. Why instead of overriding the padding for non-popover usage, do the opposite. No padding built-in and override the styles for the popover case only (which is the special case for me)

Fair pushback, I suppose I didn't fully explain why I prefer this. It has to do with the following:

The date component adds a horizontal scrollbar if the popover is smaller than 255px + 2x13px added further up that hierarchy. The min-width for popovers is 260px, defined elsewhere. Which leaves us two ways to avoid a horizontal scrollbar when used outside of the sidebar:

  1. Add padding left and right. This increases the effective width of the popover to make room for the 255px + 2x13px, without having to chagne the minimum popover width.
  2. Change the minimum popover width to 270 or 280px.

I can't stress how ugly the code for the date time library is — you kind of have to inspect it to understand why. I mean a third option is to add additional CSS classes or wrappers for any third party that means to use the component inside the popover, and set that class to 280px width. But it's hack upon hack.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 15, 2019

I've used this branch and applied this commit 53d2a09 to invert the application of the padding. I'm trying to understand what breaks?

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 15, 2019

I've used this branch and applied this commit 53d2a09 to invert the application of the padding. I'm trying to understand what breaks?

This happens:

Screenshot 2019-11-15 at 14 27 15

That is how the date-time component looks if you don't apply the following CSS:

.edit-post-post-schedule__dialog .components-datetime {
    padding: 8px;
}

In other words, if the date time component is used in a popover outside of edit-post-post-schedule__dialog, it will have a horizontal scrollbar. Unless every time it's used in that configuration, it has an additional wrapping div with 8px padding applied.

Sure, that's an option — but it feels like it optimizes the date time component to be used inline, vs. used in a popover, and I would suggest the latter is far more likely.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 15, 2019

I may be missing something obvious, if that's the case I do apologize. I was up at 5am this morning.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 15, 2019

Ok, I understand better, sorry for the back and forth.

I think all components in @wordpress/components should be optimized to be used inline. I see the popover as the exception.

That said, I checked deeper and yeah, the issue is the negative margins. I think we can probably remove them and find a way to remove the big padding that calendar has but I'm fine with this PR making an exception to the rule above.

jasmussen added 2 commits Nov 1, 2019
The popover inherited a min-width of 260, which is insufficient, then overrode it just for the edit-post-schedule flow. This means the datetime component looks fine in the sidebar, but no-where else.

This PR changes the base inherited minwidth to 270, so the component can be used outside of the sidebar.
The DateTime component as it exists, references a 3rd party library that uses lots of inline styles and hard-coded widths and dimensions.

Having explored various other approaches now, there are only two ways to fix this.

1. Actually add the padding to the component itself. This is the only way to make it work inside popovers when used outside of WordPress.
2. Do a major refactor to include a different datetime library.

In order to move forward, I'm therefore embracing the padding.
@jasmussen jasmussen force-pushed the fix/date-component branch from 36a0ee7 to dccda57 Nov 18, 2019
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Nov 18, 2019

Rebased to see if that makes the checks pass.

Thanks so much for the reviews, Riad. Every question you've asked is right, and honestly I wanted to address them. But the way the datetime component is built, it really deserves a bigger refactor to make this all possible. The problem with those negative margins is that it's a horizontally sliding calendar sheet that's built in a roundabout way. If we tinker with the margin, this sliding effect breaks when you press "next", which means the styles that are set inline need to be overridden also, and at some point we've reimplemented the whole thing in overriding CSS 😭 — it's a castle made of sand.

But I do believe there is a ticket to revisit the datetime component entirely, that will be a good opportunity to make this better.

@jasmussen jasmussen merged commit 6ecd59b into master Nov 18, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the fix/date-component branch Nov 18, 2019
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.