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

Block Editor: Fix overflowing `LinkControl`. #20154

Merged
merged 3 commits into from Feb 24, 2020

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Feb 10, 2020

Improves #20146.

@epiqueras epiqueras added this to the Gutenberg 7.5 milestone Feb 10, 2020
@epiqueras epiqueras requested a review from aduth Feb 10, 2020
@epiqueras epiqueras requested review from ellatrix and youknowriad as code owners Feb 10, 2020
@epiqueras epiqueras self-assigned this Feb 10, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 10, 2020

This seems to have the effect of making the input much narrower than it was previously in larger viewports:

Before After
Before After

Any way we can retain the width?

@epiqueras epiqueras force-pushed the fix/overflowing-link-control branch from 451cb8e to e4a911a Feb 10, 2020
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 10, 2020

Yes, a media query. Updated!

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 10, 2020

I think this helps make it less likely to overflow, since the natural width of the popover will be much narrower, but it's still possible for overflow to happen, at least on some smaller devices like iPhone SE.

image

Conversely, on larger mobile devices, there's a lot of extra space available, and the input feels unnecessarily small.

image

For me, an ideal solution would be one in which the component maximizes the space available, up to a point. For example:

width: 90vw;
max-width: $modal-min-width;

The trouble is that the popover implementation is rather naive in how it tries to reposition when space is not available. Currently, it will only ever try to flip the desired position, which means for something which occupies most of the width of the viewport, it's quite likely it's still going to overflow after the flip. I think it would be nice if the popover was implemented to "push itself away" from the edge of the screen instead.

There's also another issue here: The width of the suggestions have their own separate width, so the popover can suddenly widen after entering a search term. The widened popover can stretch beyond the viewport, even in larger devices like iPhone X.

image

I believe the intention is that the popover shouldn't ever resize after it's already visible. At least, this is how it behaves on larger displays in master.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Feb 10, 2020

I think it currently only exists in the G2 UI branches, but perhaps the same method of keeping the block toolbar from going off-screen could be used here.

@epiqueras epiqueras force-pushed the fix/overflowing-link-control branch from e4a911a to bd256c9 Feb 10, 2020
@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 10, 2020

@aduth The latest update should handle those cases better.

@aduth aduth modified the milestones: Gutenberg 7.5, Gutenberg 7.6 Feb 12, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2020

For me, that regresses back to what it was in #20146.

image

Which, from my perspective, would make sense following the change. I could have been clearer, but in my previous comment I meant to imply that it would be ideal if we could express it this way (at least as far as expressing the desired width of the popover), but that the current implementation of popover cannot support this because its implementation of repairing overflow is currently rather naive.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 12, 2020

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2020

It would never overflow if it could be guaranteed to be centered on the screen. It likely depends where horizontally in a line of a paragraph the dialog is opened, since the behavior of that dialog is to open centered at wherever the selection is, which can be (but isn’t always) aligned to the center of the viewport.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 12, 2020

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2020

It's slightly better, in that the natural width will be slightly narrower on smaller viewports (360px -> 337px on iPhone X). And it scales relative to the viewport size, which helps. The overflow still happens though.

I would be okay to merge it as an improvement, but I'd not mark it as fixing #20146.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 12, 2020

Yeah, there are still edge cases because of the popover flip. I've updated the description.

This PR is still needed even if we fix the popover.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 12, 2020

I might wonder if this would negatively impact the reusability of this component. Specifically thinking about #18061. LinkControl should not be assumed to be rendered in a popover (as of #19638), so constraining its width might not be how we'd want it to behave in other contexts (e.g. #18061 (comment)). I think it would be fair to apply this styling when it is the case that LinkControl is rendered within a popover, but I'm not sure where these styles should live. It's the sort of thing I was wondering in #18061 (comment) with my comment of "Could URLPopover be repurposed to render the stylized LinkControl".

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 12, 2020

Good points. I've scoped the styles.

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
text-overflow: ellipsis;
max-width: 230px;
overflow: hidden;
white-space: nowrap;
Comment on lines 153 to -150

This comment has been minimized.

Copy link
@aduth

aduth Feb 12, 2020

Member

The three styles text-overflow, overflow, and white-space are pretty directly linked, if I understand it correctly. I would expect that either all should be removed, or all should remain.

What was the purpose for removing white-space? As best I can tell, in combination with the other styles, it would not been the case that the element would stretch, and instead it would be truncated.

image

Contrast that with how it behaves after these changes:

image

I think either could work, but it seems like the behavior of the former was likely a conscious design decision to keep the heights of these suggestions consistent (cc @getdave).

I'd guess this might have been related to my comment at #20154 (comment) ? It feels to me we shouldn't want there to be an explicitly assigned max-width here, at least not one with a fixed pixel value. I'd guess this also hurts reusability of the component, since it seems pretty focused on it being rendered in the popover.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 12, 2020

Author Contributor

I removed it because the header still stretches up to 230px with it.

This comment has been minimized.

Copy link
@getdave

getdave Feb 17, 2020

Contributor

Yes it was deliberate to ensure consistent height

Sorry i dont get these pings. I need better GH skills!

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 17, 2020

Author Contributor

It's not working for me in Chrome. The content is cut off without an ellipsis.

@@ -3,6 +3,12 @@ $block-editor-link-control-number-of-actions: 1;
.block-editor-link-control {
position: relative;
min-width: $modal-min-width;

.components-popover__content & {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 13, 2020

Contributor

Why the LinkControl should be aware that. it can be used in a Popover?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 13, 2020

Author Contributor

Are you saying the parent component should apply a custom class name?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 13, 2020

Contributor

I don't know but it feels like:

  • The LinkControl shouldn't be aware of a Popover and should behave properly no matter where it's rendered (fluid maybe)
  • The Popover shouldn't be aware of its children of course but has smart logic to adapt to the viewport.

Now, I don't know the details of this issue to argue exactly what the fix should be.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 13, 2020

Author Contributor

The LinkControl shouldn't be aware of a Popover and should behave properly no matter where it's rendered (fluid maybe)

Agreed, this PR furthers that.

The Popover shouldn't be aware of its children of course but has smart logic to adapt to the viewport.

This requires a bigger refactor of popovers.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 13, 2020

Contributor

Agreed, this PR furthers that.

It doesn't seem like it since there's an explicit mention of popover in the link control style.

This requires a bigger refactor of popovers.

The popover should already have the logic to adapt the width to the viewport built-in. Maybe there's a bug there.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 13, 2020

Author Contributor

It doesn't seem like it since there's an explicit mention of popover in the link control style.

Which is used to remove pixel-based dimension constraints.

The popover should already have the logic to adapt the width to the viewport built-in. Maybe there's a bug there.

Yeah, that logic is not very robust and it overflows the screen in most cases.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 13, 2020

Contributor

Which is used to remove pixel-based dimension constraints.

Yes, this is cool but can't we do it consistently without any mention of the popover class. I'm thinking the LinkControl can also be shown in narrow contexts independent of the Popover being present or not.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 13, 2020

Author Contributor

No, because the way we can dampen the effect of the popover bug here does not necessarily work in other narrow contexts. We can only assume a 90vw is OK because the popover renders on top of everything.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 14, 2020

Contributor

Do you think a generic solution based on "react-resize-aware" would work here like the Placeholder component?

This comment has been minimized.

Copy link
@epiqueras

epiqueras Feb 14, 2020

Author Contributor

Yes, or react-spring/react-use-measure.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 17, 2020

@aduth This is still an improvement over master, right? Can we merge this and fix the Popover issue separately?

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 18, 2020

Why don't we try to fix the issue once and for all by making the component responsive?

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 18, 2020

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Feb 24, 2020

I'm merging this PR as it seems a simple enhancement that can be included in WordPress 5.4. As a follow up I guess we can use a "react-resize-aware" or react-use-measure based approach.

@jorgefilipecosta jorgefilipecosta merged commit 598b24e into master Feb 24, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/overflowing-link-control branch Feb 24, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 25, 2020

With regards to the discussion here about the tooltip's overflow mechanics, I think it might be worth considering to pull in Popper to replace our implementation. I've been a bit resistant to it when brought up previously, in part because our custom implementation has been mostly sufficient for our purposes, and we don't need all of what the library brings with it (i.e. avoid inflating bundle size).

Notably, it does quite well in this regards of "pushing away" from the bounds of a container:

push-away

https://popper.js.org/docs/v2/modifiers/arrow/


There are two other things to follow-up on here:

  1. My comment at #20154 (comment) alluded that if white-space is removed, then we probably should also remove the related text-overflow and overflow styles.
  2. Per @getdave's comment at #20154 (comment), we now no longer have the consistent heights intended with the link suggestions.

Issue: #20446

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Feb 25, 2020

Yes, I also think this is a place for a third party library.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 26, 2020

I've used Popper in a personal project. I think it's great but it won't replace our Popover entirely, it's something we can use to remove some custom logic from Popover and use it as a low-level abstraction.

We also have very custom behaviors (toolbar moving when it hits the edge of the editor canvas) that will most likely need to be implemented as "custom modifiers" for Popper.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Tightening Up
  
Awaiting triage
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.