-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Make the modal height and width determined by the content #9973
Conversation
Very cool work. Nicely done! A few things. The close button is a bit rectangular: It would be nice if it were square. Given the header box is a flex container, I suspect either padding on the container or margin on the child can address this. Secondly — I would love us to add some max-height to the box. I like 70%, but I could be convinced of more. 80% looks pretty good: Here's 90%: Remember that any max-height you add should be wrapped in At this height, the width sort of seems small on big screens. Not for this PR, but would it be possible and/or nice if the width was content aware just like the height is? Something about a min and max-width? Finally, it's nice to see that this PR solves the issue where the tooltip is showing on initial open. I noticed an issue in this branch, though, which is also an issue in This is not an issue in #9900: What do you think the best approach is here? |
Thanks, will check the points you've raised. Re: the max-height: there is one, see https://github.com/WordPress/gutenberg/pull/9973/files#diff-3c711058f2b216e67a13e12834045ccfR35 I can certainly adjust the value to 90% but that would be an opinionated value, i.e. a "magic number" 😉 Wouldn't be better to subtract to the full available height soem spacing based on the 8p grid ? Also, any suggestion about the |
That's a good argument. Using some screen rulers, it would feel balanced if we were able to apply about 56px margin above and below the modal. What do you think? Width was a spur of the moment inspiration. No need to address in this PR. But I would suggest 640px would be a nice max-width, and 360px a nice min-width. |
Right. I think this happens because the keyboard shortcuts modal has only one focusable element: the X close button at the top. When you click in the middle of the content, technically there's no focused element and |
Technically I think Whether it should be doing something? I'm not clear what that might be.
|
There's a |
@jasmussen I've set the 56 pixels, a min-width, and I think I've fixed the button size. Re: the modal width, I'm not sure setting a Re: the 360 pixels min-width, maybe it could be moved to the 480 pixels breakpoint? |
Maybe I'm not understanding. Here's the steps I'm taking:
A tab at this point will emit a By the looks of it, the classic editor link modal gets around this by adding |
Ahhh yes it's the tabindex, sorry for the confusion. Yep, most of the modals in core have a tabindex -1 or 0. I think we should try -1. Also, modern browsers behavior can be confusing sometimes because, regardless of where the focus landed (e.g. when body becomes the activeElement) they have implemented a |
Any progress on this one? It would be very nice to have this addressed in 4.0. |
@jasmussen there are a few points that need feedback:
|
If this fixes the issue of being able to tab through the underlying stuff, go for it.
I don't disagree. But I also think this can happen separately. Perhaps we should shelve any width related enhancements for now so we can ship this PR?
On mobile breakpoints all modals are 100% width and height anyway, right? No strong opinion here otherwise. |
4ebedcb
to
cee34c3
Compare
@jasmussen I've adjusted the top and bottom spacing to 56 pixels and added @aduth |
That is disappointing. Help me understand what makes this approach superior to the method used in #9900 which seems far simpler on the face of it? |
@jasmussen if you mean the |
cee34c3
to
7652b22
Compare
I'm unclear about how the tabbing working now. Since #10174 was merged, there aren't any issues tabbing with content inside the modal in Safari/Edge/IE 11 now, correct? I checked this out locally and it seems to work quite nicely for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here looks good to me and I'm happy with how it works when tested locally.
I'm cool with this given I can get clarification on any tabbing issues that still exist (or if they've been fixed). @afercia @aduth: could one of you clarify:
- are there still tabbing issues?
- how can they be reproduced?
A comment on the original PR: I'm also fine with IE 11 not scrolling the header.
With recent changes, the tooltip shows up on initial open. Was this intentionally removed, or is there a way we can add this back? |
Todo: explore a way to allow developers to easily set the modal width. Historically, this has been a recurring issue in core, especially with the legacy thickbox. As I see it, the best option to empower developers to easily customize the modal width would be making it dependent on the width of the content passed to the modal. This should be clearly documented though. |
That's a good idea, but worth a separate issue. I'm just rebasing this on top of master as some other changes were made to the modal styles, but after that I think this is good to merge. 👍 |
48015b4
to
1ec7073
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After rebasing this on top of @jasmussens' recent updates to the modal I'm happy with it and think it's solid. Thanks a bunch for this one, once Travis is green I'll get it merged in for 4.0.
1ec7073
to
3102ba4
Compare
The Modal component sizing can be improved a bit. Right now, on bigger screens, the height is always a fixed value of
70%
. This is far from ideal when the modal content is very short, as the screenshot on #8561 (comment) clearly illustrates.As a plugin developer, I would like to be able to manage the modal size according to my needs, with minimal effort. I'd like the height to be determined by the actual modal content, as I have no reasons to want a modal that is taller than the content. With regards to the width, I'd like to set it on the content I pass to the modal and implement my own media queries (if necessary) in my component.
This PR is a POC trying to go in that direction.
Screenshot with short content:
Screenshot with long content:
The wrapper with
overflow: auto
is now the modal frame: it already as an ARIA role and it's properly labelled. There's no need for new roles and labels as done in #9900While this approach doesn't solve all the issues, it's probably simpler and worth exploring. It also makes developers able to manage the modal sizing. I'd propose to consider to explore how to unify this new approach with the improvements currently proposed in #9900 and #9410.
Note on the header: it uses
position: sticky
which doesn't work on IE 11. Honestly, I wouldn't be so worried by the header that scrolls in IE 11. As said, this is a POC and any improvements are very welcome. /Cc @jasmussenFixes #8561