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

[Discover] Deangularize timechart header #66532

Merged

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented May 14, 2020

Summary

Fixes #65432

Checklist

Delete any items that are not applicable to this PR.

Screenshots

Before:
screenshot_before

Now:
Screen Shot 2020-05-18 at 08 45 41 AM

@stratoula stratoula self-assigned this May 14, 2020
@stratoula stratoula added Feature:Discover Discover Application v8.0.0 labels May 14, 2020
@stratoula stratoula force-pushed the discover/deangularize-datetime-histogram branch from afdd1c8 to 7f03a83 Compare May 14, 2020 10:07
@stratoula stratoula force-pushed the discover/deangularize-datetime-histogram branch from a0fc6cd to 2785e5a Compare May 14, 2020 13:24
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula marked this pull request as ready for review May 15, 2020 08:45
@stratoula stratoula requested a review from a team May 15, 2020 08:45
@stratoula stratoula added release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure labels May 15, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@stratoula stratoula requested review from cchaos and a team and removed request for cchaos May 15, 2020 09:58
@stratoula
Copy link
Contributor Author

@elasticmachine merge upstream

@stratoula stratoula force-pushed the discover/deangularize-datetime-histogram branch from ffeaaa5 to 9782531 Compare May 18, 2020 08:27
@stratoula stratoula requested a review from kertal May 18, 2020 12:28
Copy link
Member

@kertal kertal 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 👍 , tested locally in Chrome, Firefox, Safari, works as expected, no regression detected. Added a cleanup suggestion. Thx for paying attention to details like the missing warning icon. Those angular directives vanished without any warning.

@stratoula
Copy link
Contributor Author

stratoula commented May 18, 2020

@kertal thanks for the suggestion. Will wait for the design team approval and merge it!!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM! Sorry about breaking those tests. The append works well now, I think. Thanks!

@kertal
Copy link
Member

kertal commented May 18, 2020

@cchaos I've one question about wording, since I'm not a native speaker
Bildschirmfoto 2020-05-18 um 14 08 02

Hourly -> Hour, Daily -> Day, ...

right?

Maybe it would make sense to add the "per" prefix into the select box?

Auto
Per millisecond
Per second
....

In English & German, it know it's similar, per {variable}, but there could be languages where it's e.g. {variable} per

@cchaos
Copy link
Contributor

cchaos commented May 18, 2020

Ah yes, good catch. I don't think we should move the "per" to be part of the selection. I think we should try to make sure the selections themselves are all similarly phrased.

Looping @KOTungseth in here.

Should we change all of the selections to not have the "ly" affix? So it reads like:

  • per [millisecond]
  • per [day]
  • per [hour]
    ...etc

@KOTungseth
Copy link
Contributor

I agree with @cchaos that we should remove ly from the time filter options. From what I've been taught, this list (traditionally) should appear as:
Auto
Millisecond
Second
Minute
Hour
Day
Week
Month
Year

@stratoula
Copy link
Contributor Author

Thank you @KOTungseth, will proceed with the change!

@stratoula stratoula requested a review from a team as a code owner May 19, 2020 13:38
@stratoula stratoula force-pushed the discover/deangularize-datetime-histogram branch from 3a0ef58 to b535961 Compare May 19, 2020 15:54
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@streamich streamich left a comment

Choose a reason for hiding this comment

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

AppArch code changes LGTM.

@stratoula stratoula merged commit 42d21bc into elastic:master May 20, 2020
@stratoula stratoula deleted the discover/deangularize-datetime-histogram branch May 20, 2020 07:29
stratoula added a commit to stratoula/kibana that referenced this pull request May 20, 2020
* Deangularize timechart header

* Add label attr to options

* Watch the interval prop change

* Tweaking the UI

Mainly moved interval notice to an `append` and copy updates

* Remove outdated i18n tokens

* fix functional test

* Change functional test due to dom changes

* fix functional test

* remove unecessary translation

* remove watcher as it is not necessary anymore

* change interval options copies

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
stratoula added a commit that referenced this pull request May 20, 2020
* Deangularize timechart header

* Add label attr to options

* Watch the interval prop change

* Tweaking the UI

Mainly moved interval notice to an `append` and copy updates

* Remove outdated i18n tokens

* fix functional test

* Change functional test due to dom changes

* fix functional test

* remove unecessary translation

* remove watcher as it is not necessary anymore

* change interval options copies

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: cchaos <caroline.horn@elastic.co>
gmmorris added a commit to gmmorris/kibana that referenced this pull request May 20, 2020
* master: (33 commits)
  [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879)
  [Discover] Deangularize timechart header (elastic#66532)
  [Discover] Improve and unskip a11y context view test (elastic#66959)
  [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864)
  docs: update RUM documentation link (elastic#67042)
  [QA] fixup coverage ingestion tests. (elastic#66905)
  [Metrics UI] Add support for multiple groupings to Metrics Explorer (and Alerts) (elastic#66503)
  [Metrics UI] Add sorting for name and value to Inventory View (elastic#66644)
  [Metrics UI] Change Metric Threshold Alert charts to use bar charts (elastic#66672)
  [Uptime] Use React.lazy for alert type registration (elastic#66829)
  [Reporting] Consolidate API Integration Test configs (elastic#66637)
  Allow histogram fields in average and sum aggregations (elastic#66891)
  Fix saved object share link (elastic#66771)
  move role reset into the top level after clause (elastic#66971)
  Automate the labels for any PRs affecting files for the Ingest Management team (elastic#67022)
  [SIEMDPOINT] Move endpoint to siem (elastic#66907)
  server.uuid so is not used (elastic#66963)
  Revert "[ci/stats] fix git metadata collection (elastic#66840)"
  [Uptime] Unmount uptime app properly (elastic#66950)
  [Visualize] Bar chart: Show missing values on chart setting (elastic#66375)
  ...
jloleysens added a commit to jloleysens/kibana that referenced this pull request May 20, 2020
…ent/add-support-in-url-for-hidden-toggle

* 'master' of github.com:elastic/kibana: (49 commits)
  [Uptime] Improve responsiveness details page (elastic#67034)
  skip flaky suite (elastic#66669)
  Revert "Integration of a static filesystem for the node_modules (elastic#47998)" (elastic#67124)
  Support api_integration/kibana/stats against remote hosts (elastic#53000)
  chore(NA): add module name mapper for src plugins on x-pack (elastic#67103)
  Change the error message on TSVB in order to be more user friendly (elastic#67090)
  [kbn/optimizer] poll parent process to avoid zombie processes (elastic#67059)
  [Visualize] Lazy load default editor, fix duplicated styles (elastic#66732)
  Bump styled-component dependencies (elastic#66611)
  Bump react-markdown dependencies (elastic#66615)
  Fix Core docs links (elastic#66977)
  Timelion graph is not refreshing content after searching or filtering (elastic#67023)
  Remove `--xpack.endpoint.enabled=true` from README.md file (elastic#67053)
  Move apm tutorial from apm plugin into apm_oss plugin (elastic#66432)
  [Logs UI] Restore call to `UsageCollector.countLogs` (elastic#67051)
  Remove unused license check result from LP Security plugin (elastic#66966)
  [Saved Objects] adds support for including hidden types in saved objects client (elastic#66879)
  [Discover] Deangularize timechart header (elastic#66532)
  [Discover] Improve and unskip a11y context view test (elastic#66959)
  [SIEM] Refactor Timeline.timelineType draft to Timeline.status draft (elastic#66864)
  ...

# Conflicts:
#	x-pack/plugins/index_management/__jest__/client_integration/helpers/home.helpers.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Deangularize histogram dateTime range text and interval selection
7 participants