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

Allowing context menu panels to have different widths #1173

Merged
merged 7 commits into from
Sep 11, 2018

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Sep 7, 2018

Summary

Just add another key called width with a number (to be translated into px values as the style prop does) and it will force that width on the panel item itself allowing the context menu to update itself.

Note: The width transition doesn't work yet

Checklist

  • This works well on mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Proper documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes or labeled appropriately
  • Jest tests were added to match the most common scenarios
    - [ ] This was checked against keyboard-only and screenreader scenarios (accessibility not altered but may not have been perfect before)

@cchaos
Copy link
Contributor Author

cchaos commented Sep 7, 2018

Ugh, hold off on the review for a bit, I'm having a slight issue with overflow

cchaos added 4 commits September 7, 2018 16:52
- No longer watch for width
- Mobile fix
- Adding to jest test
@cchaos
Copy link
Contributor Author

cchaos commented Sep 7, 2018

Ok this can be reviewed, but I could use some help trying to get the width transition to work.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 10, 2018

I'm inclined to just leave off the width transition. The height one is so quick anyway, that it's hardly noticeable without it. The major issue being solved here is the ability to have context panels of differing widths.

@chandlerprall
Copy link
Contributor

I'm concerned that fixed widths aren't going to scale well, or won't allow future functionality. As a tangible use case, as we introduce localized content it's very probably the proper width will differ.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 10, 2018

@chandlerprall I'm not sure I agree that localization will cause widths to change. Text line-length shouldn't be determining the sizing of panels. It should be the overall content. I.e., if there's a form, you probably want the panel to be as wide as the default form width.

This is just like adding style={{ width: 400 }} on the content inside a regular popover. But since context menu panels don't respect that inner content width, we need to be able to supply it to a higher level component, and this is the way it needs to happen.

@snide
Copy link
Contributor

snide commented Sep 10, 2018

@chandlerprall @cchaos As long as it has one way to expand (height right now) it should be fine regardless of text length as long as the width transition is removed. The height will be calculated correctly as long as the width transition is off (which is how @cchaos has it now). It'll just keep growing as it needs, which is no different than how text would operate on mobile or something.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 10, 2018

👍 / 👎 ?

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.

Works for what we need it to do.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 11, 2018

I'm going to merge so I can unstuck #1137 , if we find a better way to handle this down the road, we can change.

@cchaos
Copy link
Contributor Author

cchaos commented Sep 11, 2018

jenkins, test this

@cchaos cchaos merged commit 6558b71 into elastic:master Sep 11, 2018
@cchaos cchaos deleted the context-menu-width branch September 11, 2018 16:11
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.

posthumously approving; I had pulled down and verified it works locally, code LGTM

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.

3 participants