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

Jump to bottom

[EuiSplitPanel] New component #4539

Merged
merged 20 commits into from
Mar 8, 2021
Merged

[EuiSplitPanel] New component #4539

merged 20 commits into from
Mar 8, 2021

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Feb 17, 2021 •
edited
Loading

Originally created in #4449 , but was asked to make it an exportable component, all for @snide

EuiSplitPanel is basically just a composed component of an outer and any number of inner EuiPanels with some hard-lined props, but still customizable. This was also an attempt to use a component pattern described in #4486.

Screen Shot 2021-03-02 at 15 56 35 PM

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • [ ] Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

cchaos added 6 commits February 17, 2021 13:49
Adding EuiSplitPanel (using render props… meh)
e8bb6b5 
# Conflicts:
#	src-docs/src/components/guide_section/guide_section.js
#	src-docs/src/components/guide_section/guide_section_parts/guide_section_example.tsx
#	src-docs/src/services/playground/playground.js
#	src-docs/src/views/utility_classes/utility_classes_section.tsx
Changing the split panel behavior
24c85a0 
# Conflicts:
#	src-docs/src/components/guide_section/guide_section_parts/guide_section_example.tsx
#	src-docs/src/views/utility_classes/utility_classes_section.tsx
Added docs example section
2de92ff
Changing again the way the component is setup
3ac66ca
Adding direction and responsive props
efab3a1
Update docs text
1286875
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@cchaos
Copy link
Contributor Author

cchaos commented Feb 18, 2021

@thompsongl & @chandlerprall , Leaving this one in draft mode, but curious your take on the component setup.

@thompsongl
Copy link
Contributor

I've often seen this namespace pattern before, but have never written or worked with a project that used it. It seems like the pattern lends itself well to this use case. That is, these components 1) are simply stylistic configurations of existing components, 2) are codependent, 3) are stateless and lack functionality, 4) and can be arbitrarily nested and composed.

Also, TypeScript solves one of my former hesitations with the pattern in that we'll get IDE help for what components are available in the EuiSplitPanel namespace.

So that leaves two questions:

  1. Do we want to introduce another component pattern?
  2. Are we ok with inherent naming (inner vs. outer) being the clue for composition? (as opposed to render props where it's explicit that the render param component will be inside the parent)

Less concerned about 2, because reading documentation is kind of required regardless.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2021 •
edited
Loading

Thanks @thompsongl ! I agree with all your points as well especially the TS one where it's really easy to get the right component naming and structure.

Do we want to introduce another component pattern?

It's hard to say no to this because the tech will always be changing and so it'll be hard to stick to old patterns if new and/or better ones emerge. This component is also new and pretty simplistic that if we do get negative feedback we could change it without too much side-effects.

Are we ok with inherent naming (inner vs. outer) being the clue for composition?

For this particular one, absolutely. I'm not sure what better naming there would be because it's literally about visual and code composition.

I think the final consideration is how to handle this type of component within the Props table. I'm not seeing a way to explicitly name each table for the .Inner vs .Outer.

@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2021

I think the final consideration is how to handle this type of component within the Props table. I'm not seeing a way to explicitly name each table for the .Inner vs .Outer.

I think I actually figured out a way to handle it via the __docgenInfo.description.

@cchaos cchaos marked this pull request as ready for review March 2, 2021 20:51
@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2021

Opening this up for review now. I think we're pretty happy with the format and we'll see what kind of feedback we get.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@cchaos cchaos requested review from thompsongl and elizabetdev March 2, 2021 21:21
@cchaos
Copy link
Contributor Author

cchaos commented Mar 2, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

thompsongl
thompsongl suggested changes Mar 4, 2021
View reviewed changes
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Code LGTM. Just a couple small things.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

elizabetdev
elizabetdev approved these changes Mar 8, 2021
View reviewed changes
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested in Chrome, Edge, Firefox and Safari. It looks good! 🎉

@cchaos
Copy link
Contributor Author

cchaos commented Mar 8, 2021

Jenkins, test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_4539/

@cchaos cchaos merged commit cdff95e into elastic:master Mar 8, 2021
@cchaos cchaos deleted the new/split_panel branch March 8, 2021 21:49
@dependabot dependabot bot mentioned this pull request Mar 15, 2021
Closed
@chandlerprall chandlerprall mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thompsongl thompsongl thompsongl approved these changes

@elizabetdev elizabetdev elizabetdev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cchaos @kibanamachine @thompsongl @elizabetdev