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

Attempting to fix with EuiFlyout maxWidth #2125

Merged
merged 8 commits into from
Jul 20, 2019

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jul 15, 2019

Fixes #2134 (kind of)

Really all I did was change the docs example to a) not be wrong, b) showcase all the permutations to show what works and what doesn't.

Screen Shot 2019-07-17 at 14 32 22 PM

But I also:

  1. Rounded the SASS calculations so they don't come out as decimal points
  2. Reduced the min width of the medium size so that it just fits our form width
  3. Fixed the small flyout sizing on mobile

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • [ ] This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • [ ] This was checked for breaking changes and labeled appropriately
  • [ ] Jest tests were updated or added to match the most common scenarios
  • [ ] This was checked against keyboard-only and screenreader scenarios
  • [ ] This required updates to Framer X components

@thompsongl
Copy link
Contributor

Does anyone have any thoughts or other experiments to try?

The only thing I can think of is to get more JS involved :ducks:
We'd need to do some math on the min and custom max values provided to make sense of which is more accurate.
We already use a style tag with a prop value, so we're part of the way there

const value = typeof maxWidth === 'number' ? `${maxWidth}px` : maxWidth;
newStyle = { ...style, maxWidth: value };

I'm not convinced this is a good idea yet

@chandlerprall
Copy link
Contributor

I think, generally, the current functionality is better (minWidth wining); A flyout's content should be important enough to demand the greater screen real estate.

That said, I'm sure there are valid use cases for restricting the width smaller than the minWidth, but I feel that should be an opt-in case - perhaps we provide a prop that sets min-width back to auto?

@snide
Copy link
Contributor

snide commented Jul 16, 2019

@cchaos I'm OK with minwidth winning as well. It might be unexpected, but it's generally going to be "more right most of the time".

If we felt extremely strong about it, we could just add a size="custom" or size="fluid' that also requires maxWidth to be set

@cchaos
Copy link
Contributor Author

cchaos commented Jul 17, 2019

Ok, I updated the summary of the PR to align with the actual changes in this PR. (Moved the original summary to an issue for tracking).

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Code & docs changes LGTM

Copy link
Contributor

@snide snide left a comment

Choose a reason for hiding this comment

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

LGTM. Small suggestion for your 404 change.

src-docs/src/views/not_found/not_found_view.js Outdated Show resolved Hide resolved
cchaos and others added 7 commits July 20, 2019 00:11
Also fixed up 404 page
..to accomodate forms.

Also added docs example for this forms width trick.
Co-Authored-By: dave.snider@gmail.com <dave.snider@gmail.com>
@cchaos cchaos merged commit d5233c3 into elastic:master Jul 20, 2019
@cchaos cchaos deleted the flyout-max-width branch July 20, 2019 04:39
@snide snide mentioned this pull request Jul 22, 2019
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EuiFlyout maxWidth can be wrong
4 participants