[Graph] Settings EUI-ification#44587
Conversation
💚 Build Succeeded |
💚 Build Succeeded |
💚 Build Succeeded |
kertal
left a comment
There was a problem hiding this comment.
Code LGTM 👨🎨. just minor remarks. There's one thing to investigate. After setting the Sample Size to 100. I clicked on the magnifier to redo the search, but the sample size didn't change (2000). Tested in Chrome
x-pack/legacy/plugins/graph/public/components/graph_settings/url_template_form.tsx
Outdated
Show resolved
Hide resolved
| * | ||
| * @returns A lookup function taking an item of the list supplied to the | ||
| * hook and returning the id for the given item. | ||
| */ |
| /** | ||
| * This is a helper to tie state updates that happen somewhere else back to an angular scope. | ||
| * It is roughly comparable to `reactDirective`, but does not have to be used from within a | ||
| * template. |
| <EuiFieldNumber | ||
| fullWidth | ||
| min={1} | ||
| step={1} |
There was a problem hiding this comment.
I think it makes sense to define a max , although I doubt somebody is a stupid as me like me, setting the max docs to a few billons or more
would lead to a 500. There a some other numeric fields on this tab where it might make sense to define max.
There was a problem hiding this comment.
There shouldn't be a theoretical limit - Maybe you chose a field that contains multiple values in a single document?
This must be a single-term field or searches will be rejected with an error
There was a problem hiding this comment.
When you type in a number that's out of range of int (>2147483647 says elasticsearch) you get an Exception
💚 Build Succeeded |
|
@kertal Thanks for your review. Very good catch with the sample size - changed settings weren't propagated correctly if the Graph is already initialized. This should be fixed now, could you check again? |
💔 Build Failed |
|
Jenkins, test this |
kertal
left a comment
There was a problem hiding this comment.
Code LGTM, tested changes propagation, works correctly in Chrome now :)
💚 Build Succeeded |
cchaos
left a comment
There was a problem hiding this comment.
LGTM, just one suggestion for the tabs
x-pack/legacy/plugins/graph/public/components/graph_settings/graph_settings.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded |
* create graph listing page * clean up app folder * remove inline loading menu * also add badge to workspace route * fix tests * fix graph spaces functional test * generate documentation for new breadcrumb property * fix test subject names * remove unused translations * start implementing save modal flow for Graph * fix spaces functional test * wip save modal * wip save modal * add and style save modal * add placeholder to description field * disable dirty check on breadcrumb navigation and fix delete function * reactify most of settings * improve onClick typing on breadcrumb * fix newline error and use new types in dashboard app controller * EUIify drilldown * EUIify drilldown * fix translation errors * fix i18n translation for real * fix icon and work on autoreplace template logic * use switch instead of checkbox * code review * fix i18n phrases * remove fragments * add KQL encoder to be able to drill down to discover * fix flyout overflow issue * delete old settings logic * remove obsolete files * fix tests * fix i18n phrase typo * fix most translations * start implementing review comments * continue implementing review * continue cleanup of settings forms * remove unnecessary max-width and add commentary * move to async syntax * clean up implementation * use description instead of title * work on settings form * fix snapshot * fix drilldown list * add tests for use_list_keys * rename stuff and add documentation * fix translatiojns * fix some rename references * start typing saving and loading * typing persistence stuff * adress review comments and set width for all save modals * type serializing logic * fix bug and improve typing * fix classname * wip * wip * add notes * style blacklist group * remove merge marker * improve icon rendering * wip * start refactoring * remove js implementation * remove js implementation * remove duplicate file * add some tests * Design cleanup * Fix SASS files * revert style fixes in app.js * implement todos * fix errors and prematurely enabling inspect panel * deselect icon after clicking again * fix types * make icon list accessible * clean up and add some more tests * improve number handling * fix deserialization * polishing drilldown-ui * rename types * clean up imports * fix tests * add handling for canEditDrillDowns setting * propagate changes to workspace client * align tabs with other elements
Summary
This PR moves the "Advanced settings", "Blacklist" and "Drill-downs" menus into an EUI flyout.
The inspect view is not part of this because it will be rewritten to use the standard "Inspect panel" from other apps instead.
It also fixes #18027 by adding a KQL encoder for drilldown URLs and fixing the default configured drilldown.
Various supplemental files have been moved to typescript in the process.
Advanced settings
before
after
Blacklist
before
after
Drill-downs
before
after list
after form
when a auto-templatable URL is pasted
Initial text
ping @cchaos this PR is not ready yet from a technical standpoint but it's far enough to talk about how these menus should look like.
Currently the flyout opens with a click on the "Settings" menu button, initially showing the advanced settings form.
Maybe we could move the individual tabs of the flyout into separate flyouts to save a click when going into blacklist or drill-downs, but it might make the menu to busy, not sure about that.
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.For maintainers