[Vis: Default editor] EUIficate order and size controls #35134
Conversation
sulemanof
added
Team:Visualizations
|
Pinging @elastic/kibana-app |
💔 Build Failed |
💔 Build Failed |
💚 Build Succeeded |
|
Can we put them on the same line as before?:
|
Hi @cchaos ! Does it an appropriate way or you could propose another one? Thanks 😊 P.S. : if @timroes and @maryia-lapata are OK with this ? |
# Conflicts: # src/legacy/ui/public/agg_types/buckets/significant_terms.js
|
My worry with your inline style solution is if they are ever actually not used together then you're always applying that 50% width. I would suggest creating some way of allowing grouping multiple inputs on one line as it's the most extensible. Then you can just use EuiFlexGroups to contain them. |
💚 Build Succeeded |
@cchaos , during the investigation of the problem I found that another solutions would break an existing interface of building aggregation params container.
|
💚 Build Succeeded |
| import React from 'react'; | ||
|
|
||
| const wrapWithInlineComp = Component => props => ( | ||
| <div className={`visEditorAggParam--inline visEditorAggParam--inline-${props.aggParam.name}`}> |
There was a problem hiding this comment.
I think that if you go the route of renaming this to explicitly state half then you won't need to depend on the props.aggParam.name and the margin can be added to a sibling CSS selector like: .visEditorAggParam--half + .visEditorAggParam--half
There was a problem hiding this comment.
Unfortunately the solution wouldn't work because of our current structure (both them are built through the existing interface and are wrapped into additional elements). They are not siblings and that was the main pain of positioning them both since the start.
There was a problem hiding this comment.
@cchaos are you OK with current implementation ?
There was a problem hiding this comment.
I just have a concern around using the aggParam name.
Will props.aggParam.name eve have a space in it like "name here"? If so you'll end up with a className list like: visEditorAggParam--inline visEditorAggParam--inline-name here which causes .here to be a valid class which is not good.
I also still think you can change the class from --inline to --half so it's easily understood that the CSS will make that field 50% wide.
There was a problem hiding this comment.
Will
props.aggParam.nameeve have a space in it like "name here"? If so you'll end up with a className list like:visEditorAggParam--inline visEditorAggParam--inline-name herewhich causes.hereto be a valid class which is not good.
It never can have a space in a name because we are using these names in a url string. These names should always have or camelCase style or _ between words, like intervalBase, ip_range, min_doc_count, extended_bounds and so on.
I also still think you can change the class from
--inlineto--halfso it's easily understood that the CSS will make that field 50% wide.
This work has been already done in this commit
| width: 50%; | ||
| } | ||
|
|
||
| .visEditorAggParam--inline-size { |
There was a problem hiding this comment.
As stated above about the class names this can be changed to:
| .visEditorAggParam--inline-size { | |
| .visEditorAggParam--half + .visEditorAggParam--half { |
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
…_and_size # Conflicts: # src/legacy/ui/public/agg_types/buckets/terms.js
💚 Build Succeeded |
* EUIficate order and size controls
Summary
Part of #30922.
EUIfication of other
orderandsizecontrols for aggregation parameter.These controls are used under
TermsandSignificant Termsaggregations.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers