DateTime Component: Fix sizing of help info #34370
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewserong
requested review from
ajitbohra and
chrisvanpatten
as code owners
andrewserong
added
[Type] Bug
|
@jasmussen I've just added you as a reviewer since I see you've (fairly) recently done some work on this component 🙂 |
|
Size Change: +48 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
jasmussen
approved these changes
Aug 30, 2021
|
Thanks for reviewing @jasmussen, I agree, it's better for the consumers of the component to define their own requirements for layout and sizing 👍 And thanks for mentioning the mixin, it wasn't intentional skipping using that — I saw there were a couple of other I saw that there was an existing z-index layer for focused button components, so I've re-used that one instead of adding a separate one for this case. Let me know if you think it could do with tweaking further, otherwise I'll merge in this PR tomorrow 🙂 |
andrewserong
force-pushed
the
fix/date-time-picker-help-view-sizing
branch
from
3d6cbc6 to
99e25be
Compare
|
Thanks for the fix. I think it's always best practice to use the mixin, but especially in cases of "just elevate this by one" it can be okay to do without it. Mostly because z-index contexts aren't absolute and new contexts change them, the mixin makes most sense for those items that are all in the same stacking order. It seems like you rebased at a bad time, tests are failing consistently on this one for some reason. Might want to try giving it a fresh one. |
andrewserong
force-pushed
the
fix/date-time-picker-help-view-sizing
branch
from
99e25be to
aff59a8
Compare
|
Thanks for the extra context Joen, this is all super helpful as I learn about more corners of the repo that I haven't explored before 😀 I've given this another rebase and all the CI actions are passing, so I'll merge it in (looks like the issue was resolved in #34422). Thanks again! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds a couple of styling fixes to the DateTime component, to ensure that the help info view has an adequate minimum width, and that the focus styling on the Calendar Help button displays correctly.
z-indexto the buttons in the DateTime picker to ensure that when the Reset or Calender Help buttons are clicked/focused that the top border of the focus style is visibleHow has this been tested?
Manually, select the Publish date from the Status & visibility section in a post's settings panel.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.jsfiles for terms that need renaming or removal).