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

Restore min-width to popover. #9086

Merged
merged 1 commit into from
Aug 17, 2018
Merged

Restore min-width to popover. #9086

merged 1 commit into from
Aug 17, 2018

Conversation

jasmussen
Copy link
Contributor

This fixes a regression introduced as part of #8756. Basically we removed the min-width so as to let the content itself expand the popover. But alas an empty textfield, like the URL input field, did not expand the popover as it should. We could look at adding the min-width to the URL input field instead, this should expand the popover, but given the fallout this min-width removal has caused already, it's probably good to keep it in place as a baseline.

Screenshot of this being fixed:

screen shot 2018-08-17 at 14 20 41

This fixes a regression introduced as part of #8756. Basically we removed the min-width so as to let the content itself expand the popover. But alas an empty textfield, like the URL input field, did not expand the popover as it should. We could look at adding the min-width to the URL input field instead, this should expand the popover, but given the fallout this min-width removal has caused already, it's probably good to keep it in place as a baseline.
@jasmussen jasmussen added the [Type] Bug An existing feature does not function as intended label Aug 17, 2018
@jasmussen jasmussen self-assigned this Aug 17, 2018
@jasmussen jasmussen requested a review from a team August 17, 2018 12:24
@mtias
Copy link
Member

mtias commented Aug 17, 2018

Keeping as baseline sounds good to me.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jasmussen
Copy link
Contributor Author

I'm stepping away for a bit, but please feel free to merge when appropriate.

@mtias mtias merged commit a448f78 into master Aug 17, 2018
@mtias mtias deleted the fix/popover-min-width branch August 17, 2018 12:38
@aduth
Copy link
Member

aduth commented Aug 17, 2018

Mmm, the idea of not having a predefined minimum width for a popover seems sensible. Shouldn't the input specify its own minimum width if this is a desired thing?

(To clarify, this is an observation for future improvement, not a blocker on introducing a fix)

@aduth
Copy link
Member

aduth commented Aug 17, 2018

We use popovers for tooltips, for example, which shouldn't have a minimum width (and it shouldn't also be their responsible to unset any such minimum width†).

(Runs to check whether tooltips have regressed Is OK)

† Edit: As it is currently:

.components-tooltip:not(.is-mobile) .components-popover__content {
min-width: 0;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants