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.

Sign up for GitHub

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
Merged

Block Editor: Fix overflowing LinkControl. #20154

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd256c9
Block Editor: Fix overflowing `LinkControl`.
epiqueras Feb 10, 2020
f25c945
Block Editor: Scope changes to popover content.
epiqueras Feb 12, 2020
5f7c53e
Update packages/block-editor/src/components/link-control/style.scss
epiqueras Feb 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/block-editor/src/components/link-control/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 & {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

// LinkControl popover.
Expand Down Expand Up @@ -147,7 +153,6 @@ $block-editor-link-control-number-of-actions: 1;
text-overflow: ellipsis;
max-width: 230px;
overflow: hidden;
white-space: nowrap;
Comment on lines 153 to -150
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it was deliberate to ensure consistent height

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

.block-editor-link-control__search-item-title {
Expand Down