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

Design Prototype: Query builder pattern #109

Closed
wants to merge 15 commits into from
Closed

Conversation

snide
Copy link
Contributor

@snide snide commented Nov 3, 2017

Still working through this one. Code needs a cleaning

Additions

  • Adds a few new icons.
  • Adds xs size to our empty buttons (I don't want this applied to regular buttons, which is why I didn't add it there).
  • Adds xs size to horizontal rules.
  • Adds a responsive prop to EuiFlexGroup which disables responsiveness (useful when you use flexgroup more for alignment rather than gridding).
  • Adds a EuiQueryPanel and EuiQueryPanelBar component.
  • Adds a EuiStatusPill component.

Todo

  • Fix inactive focus state
  • Show the first step of the pattern, where no pills exist and only a single bar exists.
  • In a later PR, apply an auto position to popovers so they dynamically figure out the best "center" position to pop out from based on screen position.
  • Figure out whether togglepill is reusable, should be renamed or put into the query components directly.
  • In a later PR, define things like this as "patterns", which are snippets of combined components, rather than documenting individual ones.
  • Add docs for the responsive changes.

@snide
Copy link
Contributor Author

snide commented Nov 3, 2017

@cjcenizal There's still some code cleanup I can do here, but I think it's got all it needs from a pattern basis for how we handled pills. If you have a moment, could use advice on the following.

  1. I have a EuiTogglePill component for the actual pills themselves. Should I just rename these and put them into the query stuff, or do you think I should keep them out and separately document them as their own thing? I'll prolly need to name it differently either way. They're more buttons than anything.
  2. How should we handle "patterns". This code fakes a lot of stuff with state management. I could always push content into arrays and do all that kind of stuff, but at that point we're getting beyond UI. My primary want here was to show how animations and transitions should work.

@snide snide mentioned this pull request Nov 3, 2017
51 tasks
@cjcenizal
Copy link
Contributor

Should I just rename these and put them into the query stuff, or do you think I should keep them out and separately document them as their own thing?

It seems to me like something we could reuse. In which case keeping it separate with its own documentation page would be helpful.

How should we handle "patterns"

For now, I think it's OK to treat them as just another component. For example, if we have a pattern of some kind of settings panel with a lot of switches in it, we could create a "Settings" documentation page. So the way you have it now with a "Query Panel" page works well, I think. Maybe later we can add a category for "Patterns" to help differentiate these highly-composed components from the primitive ones. (On that note, I think it's safe to remove the call-out you have... we could maybe put that copy on the home page instead, but I don't think any of these examples will work in production without some customization.)

I think we should extract the Popovers into their own files so that the source code can focus more on the composition of the surface-level elements. I think pushing content into arrays and making this demo as interactive as possible will be worth it (and straightforward enough to implement). I can take care of these if you want.

@snide
Copy link
Contributor Author

snide commented Nov 3, 2017

@cjcenizal OK. Sounds good. I'll move the pill component out and document them separately, then leave the harder JS stuff for you.

@snide
Copy link
Contributor Author

snide commented Nov 4, 2017

@cjcenizal OK. I renamed the pills to EuiStatusPills (thinking is that they are pills that show some form of status) and documented them separately / made them more accessible. Made the warning more brief just to say it's a "design prototype".

The panel doc is still my designer code with fake states, but I think stylistically / functionally it's mostly there if you're up for figuring out the JS side.

Next step for me would be to figure out the date stuff, which I should do in a separate PR.

@snide snide changed the title WIP: Query builder pattern Design Prototype: Query builder pattern Nov 4, 2017
@snide
Copy link
Contributor Author

snide commented Nov 6, 2017

Went ahead and added a "filter-only" version of this after some discussions with @BigFunger and @pickypg. They think this kind of system could be flexible enough to use in their giant table lists as well.

We should definitely consider this a generic component not tied to discovery.

@snide snide added the prototype label Nov 7, 2017
@bevacqua
Copy link
Contributor

bevacqua commented Nov 8, 2017

@snide Thinking out loud here: do we want this to be in EUI or should Kibana have this kind of component in its own codebase, depending on EUI? It looks like the kind of complex control that we might want to consider keeping outside of the EUI component codebase

@cjcenizal
Copy link
Contributor

Good question, Nico. If we were to go down that route, I think we first would need to set up some way to consume EUI in the Kibana repo (but not in Kibana itself), use its SCSS and React components, dynamically define new documentation pages, and figure out how to publish those docs. I think we'd also have to get serious about documenting breaking changes (essentially any change to a React component's interface, a CSS class name, or SCSS variable/mixin name).

This seems to me like too much to take on for now, so for the sake of simplicity my vote is to keep everything in EUI for the time being.

@pickypg
Copy link
Member

pickypg commented Nov 8, 2017

Since I was mentioned: my vote would be to keep it into EUI. If ECE is going to depend on EUI, then this seems like a control that would benefit their UI in the same way that it will likely benefit X-Pack monitoring's UI.

@bevacqua
Copy link
Contributor

bevacqua commented Nov 8, 2017

Could we at least make some sort of conscious effort to keep these complex controls strictly separate from simpler components like form controls?

That way, you'd have the more conventional components on the one hand (form inputs, labels, flex groups, selects, dropdowns, icons, and so on) and on the other hand you'd have unconventional, complex components such as this one. All I'm asking for in that case would be having a src/components/complex directory where we put things like the query_panel into.

@cjcenizal
Copy link
Contributor

SGTM @bevacqua. @snide and I have been tossing around the idea of a separate "Patterns" section, which sounds similar to what you're suggesting.

@bevacqua
Copy link
Contributor

bevacqua commented Nov 8, 2017

That might fit the bill. This introduces a couple new simple components, as far as I can tell, and then the pattern would be the query panel itself, correct?

@cjcenizal
Copy link
Contributor

Yes I think so.

@snide
Copy link
Contributor Author

snide commented Nov 8, 2017

Yes. Agree with keeping it in EUI and making a new subsection for "Patterns". I think this can be a generic filter bar for any long list of items, not just Kibana.

What I would like to know, is what should we do with this stuff when it lives as a "prototype"? A lot of times I like to show these examples to the embedded teams and say... hey, this is what I was thinking. With the docs online, it makes that really easy now, I could just pass them a URL.

Basically... is this mergable if it carries the warning? If not (and that's fine), I should probably cherry pick out some of the generic items separately. Specifically all this stuff:

  • Adds a few new icons.
  • Adds xs size to our empty buttons (I don't want this applied to regular buttons, which is why I * didn't add it there).
  • Adds xs size to horizontal rules.
  • Adds a responsive prop to EuiFlexGroup which disables responsiveness (useful when you use flexgroup more for alignment rather than gridding).

Going forward I should put these as separate PRs, but a lot of this kind of stuff comes about as I'm building a new design.

@bevacqua
Copy link
Contributor

bevacqua commented Nov 8, 2017

Dave, I would say no: generally merging WIP blocks or failing tests can snowball into a pretty hard to maintain repo. I'll give you an alternative on Slack (localtunnel)

@cjcenizal
Copy link
Contributor

what should we do with this stuff when it lives as a "prototype"?

I think we should leverage the usual code review process in this case. I think most people would assume that if code is in master, it's ready for use in production. If it's in a feature branch, it's still in development. So my suggestion would be to rely on the "checkout branch / review locally / leave comments" cycle for anything that we're not prepared to ship.

Going forward I should put these as separate PRs, but a lot of this kind of stuff comes about as I'm building a new design.

This has been my experience too. It takes more time to clean up the code and cherry-pick out unrelated improvements, but it makes for a smoother development process (cleaner history and clearer code reviews).

@snide
Copy link
Contributor Author

snide commented Nov 8, 2017

@cjcenizal @bevacqua That's what I thought (and agree with). I'll cherry pick out.

@snide
Copy link
Contributor Author

snide commented Nov 13, 2017

This broke with the webpack moves. Created a new PR.

@snide snide closed this Nov 13, 2017
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.

4 participants