-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Discover] Deangularize and euificate sidebar #47559
[Discover] Deangularize and euificate sidebar #47559
Conversation
💔 Build Failed |
💔 Build Failed
History
To update your PR or re-run it, just comment with: |
@elasticmachine merge upstream |
merge conflict between base and head |
Pinging @elastic/kibana-app (Team:KibanaApp) |
e95adb6
to
59af21d
Compare
…-08-discover-sidebar-field
…-08-discover-sidebar-field
@myasonik for sure! |
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_sidebar.tsx
Show resolved
Hide resolved
…-08-discover-sidebar-field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran through the functionality of this just to make sure there are no breaks. Given the size of this PR, @kertal and I decided I'll do my PR separately after this merged. Since it's for 7.8 we've got plenty of time, but I should have it up in a day or two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hoping we can make a few a11y improvements in this PR too 🙂
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field.tsx
Show resolved
Hide resolved
const addLabel = i18n.translate( | ||
'kbn.discover.fieldChooser.detailViews.filterValueButtonAriaLabel', | ||
{ | ||
defaultMessage: 'Filter for this value', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass in what the value of "this" is so that the action can have more context?
Rendered string should ultimately read something like "Filter for currency value". (Might want to check with a copy writer on the best way to phrase that.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to Filter for {fieldname}: "{value}"
dear @gchaps , does that sound ok?
const removeLabel = i18n.translate( | ||
'kbn.discover.fieldChooser.detailViews.filterOutValueButtonAriaLabel', | ||
{ | ||
defaultMessage: 'Filter out this value', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, replace "this" with the actual thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to Filter out {fieldname}: "{value}"
dear @gchaps , does that sound ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kertal Can you please give me an example of how this message will read with values for {fieldname} and {value}?
I'm leaning more toward something that might be a littler easier to read "Filter out value for {fieldname}.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gchaps currently it would be
Exlusive Filter: Filter out extension: "php"
Inclusive Filter: Filter for extension: "php"
that would change to
Exlusive Filter: Filter out "php" for extension
Inclusive Filter: Filter for "php" for(?) extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the one on top:
Filter out extension: "php"
Filter for extension: "php"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
already none :)
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field.tsx
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field.tsx
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_sidebar.tsx
Outdated
Show resolved
Hide resolved
src/legacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_sidebar.tsx
Outdated
Show resolved
Hide resolved
...cy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field_bucket.tsx
Outdated
Show resolved
Hide resolved
...gacy/core_plugins/kibana/public/discover/np_ready/components/sidebar/string_progress_bar.tsx
Outdated
Show resolved
Hide resolved
...cy/core_plugins/kibana/public/discover/np_ready/components/sidebar/discover_field_bucket.tsx
Show resolved
Hide resolved
@elasticmachine merge upstream |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* Split angular templates into React components * Add tooltip for field label * Adapt SCSS * Cleanup angular directives * Extract helper functions * Improve tests + docs * Move css to _sidebar.scss * Exclude _id field from displaying the Visualize button to prevent an ES error * A11y improvements
* Split angular templates into React components * Add tooltip for field label * Adapt SCSS * Cleanup angular directives * Extract helper functions * Improve tests + docs * Move css to _sidebar.scss * Exclude _id field from displaying the Visualize button to prevent an ES error * A11y improvements
* upstream/master: (69 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...
* master: (64 commits) Adding PagerDuty icon to connectors cards (elastic#60805) Fix drag and drop flakiness (elastic#61993) Grok debugger migration (elastic#60658) Endpoint: Fix resolver SVG position issue (elastic#61886) [SIEM] version 7.7 rule import (elastic#61903) Added styles to make combobox list items wider for alerting flyout (elastic#61894) [UA] Tight worker loop can cause high CPU usage (elastic#60950) [ML] DF Analytics results table: use index pattern field format if one exists (elastic#61709) [ML] Catching unknown index pattern errors (elastic#61935) [Discover] Deangularize and euificate sidebar (elastic#47559) Endpoint: Add ts-node dev dependency (elastic#61884) Add an onBlur handler for the kuery bar. Only resubmit when input changes. (elastic#61901) [ML] Handle Empty Partition Field Values in Single Metric Viewer (elastic#61649) Auto interval on date histogram is getting displayed as timestamp per… (elastic#59171) [Maps] Explicitly pass fetch function to ems-client (elastic#61846) [SIEM][CASE] Fix aria-labels and translations (elastic#61670) [ML] Settings: Increase number of items that can be paged in calendars and filters lists (elastic#61842) [EPM] update epm filepath route (elastic#61910) APM] Set ignore_above to 1024 for telemetry saved object (elastic#61732) [Logs UI] Log stream row rendering (elastic#60773) ...
Summary
Deangularize and Euification of sidebar. Before this PR only, parts of the sidebar (index selection, field filter) were in React, now it's 100% 📈
Out of scope: initially we planed a redesign of the sidebar, and decided to postpone this for now (Might be part of #51531).
Old:
New:
Part of #38646 and resolves #37547
Testing
Check for regressions, since big parts where rewritten!
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.- [ ] Documentation was added for features that require explanation or tutorials- [ ] This was checked for keyboard-only and screenreader accessibilityFor maintainers
- [ ] This was checked for breaking API changes and was labeled appropriately- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately