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.

Sign up for GitHub

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

Jump to bottom

[Vis: Default editor] EUIficate date_ranges param editor #38647

Merged
merged 11 commits into from
Jun 24, 2019
Merged

[Vis: Default editor] EUIficate date_ranges param editor #38647

merged 11 commits into from
Jun 24, 2019

Conversation

sulemanof
Copy link
Contributor

@sulemanof sulemanof commented Jun 11, 2019 •
edited
Loading

Summary

Part of #30922.
Fix #38867.
EUIfication of date_ranges control for Date Range aggregation.

date_ranges

The PR includes:

  • removal of directives - validate-date-math and documentation-href;
  • removal of test suits for validate-date-math directive;
  • added test suits for DateRangesParamEditor component, which includes validation tests (previously based in validate-date-math);

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@sulemanof sulemanof added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0 labels Jun 11, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@cchaos
Copy link
Contributor

cchaos commented Jun 11, 2019

Are blank ones valid? It's behaving that way, but it seems to me they shouldnt?

Screen Shot 2019-06-11 at 13 51 14 PM

@timroes timroes mentioned this pull request Jun 12, 2019
Closed
29 tasks
@sulemanof
Copy link
Contributor Author

Are blank ones valid? It's behaving that way, but it seems to me they shouldnt?

Screen Shot 2019-06-11 at 13 51 14 PM

The behavior was not changed, also valid in previous versions:

image

If I'm not wrong, the empty values request means any existing date. Maybe it is a great opportunity to think about placeholders for empty inputs - e.x. earlier -> later - (such as was proposed in Ranges aggregation - #36558)

@cchaos
Copy link
Contributor

cchaos commented Jun 12, 2019

Maybe it is a great opportunity to think about placeholders for empty inputs

Yes please! Please work with whoever you need to to accomplish this.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof mentioned this pull request Jun 13, 2019
Closed
@sulemanof sulemanof requested a review from cchaos June 19, 2019 08:52
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

maryia-lapata
maryia-lapata approved these changes Jun 20, 2019
View reviewed changes
Copy link
Contributor

@maryia-lapata maryia-lapata left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally on Chrome, Mac.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

cchaos
cchaos approved these changes Jun 20, 2019
View reviewed changes
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 with one word change.

src/legacy/ui/public/agg_types/controls/date_ranges.tsx Outdated Show resolved Hide resolved
flash1293
flash1293 approved these changes Jun 21, 2019
View reviewed changes
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

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

LGTM with one suggestion but nothing important:
On narrow screens the config UI takes up the whole width which means there is enough space for the two date inputs to be shown beside each other (by adding the responsive prop: <EuiFlexGroup responsive={false} gutterSize="s" alignItems="center">).

IMHO it looks better that way because the date ranges don't tend to get that long

Screenshot 2019-06-21 at 08 44 28

vs

Screenshot 2019-06-21 at 08 44 03

What do you think, @cchaos ?

@sulemanof
Copy link
Contributor Author

IMHO it looks better that way because the date ranges don't tend to get that long

@flash1293 good finding, thanks! 👍

Maybe you have an info of how many people using kibana on narrow screens? (I'm just curious, seems it is not really convenient on mobile devices)

@elasticmachine
Copy link
Contributor

💔 Build Failed

@flash1293
Copy link
Contributor

Maybe you have an info of how many people using kibana on narrow screens? (I'm just curious, seems it is not really convenient on mobile devices)

@sulemanof I don't :) Looking at dashboards is probably a common use case, editing visualizations not so much I would expect. I was more thinking about people having small browser windows because they show multiple kibana tabs at once

@sulemanof
Copy link
Contributor Author

retest

@cchaos
Copy link
Contributor

cchaos commented Jun 21, 2019

Yes, good catch @flash1293 !

While we don't officially say that Visualize is mobile-friendly, we try to ensure the layout isn't completely broken. So I agree that it should add the responsive={false} prop. Eventually, we'll get to a place where our apps are more mobile-friendly.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@sulemanof
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@sulemanof sulemanof merged commit 3184be1 into elastic:master Jun 24, 2019
@sulemanof sulemanof deleted the EUIfication/date_ranges branch June 24, 2019 09:13
sulemanof added a commit that referenced this pull request Jun 24, 2019
@sulemanof
[Vis: Default editor] EUIficate date_ranges param editor (#38647)
f39a027 
* EUIficate date_ranges param editor

* Add unit tests

* Move link before the list

* Add validation

* Fix comments

# Conflicts:
#	src/legacy/ui/public/agg_types/controls/date_ranges.html
sulemanof added a commit that referenced this pull request Jun 24, 2019
@sulemanof
Revert "[Vis: Default editor] EUIficate date_ranges param editor (#38647
4db5e65 
)"

This reverts commit f39a027.
sulemanof added a commit to sulemanof/kibana that referenced this pull request Jun 24, 2019
@sulemanof
[Vis: Default editor] EUIficate date_ranges param editor (elastic#38647)
b49c671 
* EUIficate date_ranges param editor

* Add unit tests

* Move link before the list

* Add validation

* Fix comments

# Conflicts:
#	src/legacy/ui/public/agg_types/controls/date_ranges.html
@sulemanof sulemanof mentioned this pull request Jun 24, 2019
Merged
sulemanof added a commit that referenced this pull request Jun 25, 2019
@sulemanof
[Vis: Default editor] EUIficate date_ranges param editor (#38647) (#3…
d178af0 
…9505)

* EUIficate date_ranges param editor

* Add unit tests

* Move link before the list

* Add validation

* Fix comments

# Conflicts:
#	src/legacy/ui/public/agg_types/controls/date_ranges.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos approved these changes

@flash1293 flash1293 flash1293 approved these changes

@maryia-lapata maryia-lapata maryia-lapata approved these changes

@wylieconlon wylieconlon Awaiting requested review from wylieconlon

@chrisdavies chrisdavies Awaiting requested review from chrisdavies

Assignees
No one assigned
Labels
backported Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.3.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add warning for empty values in a Date Range aggregation
5 participants
@sulemanof @elasticmachine @cchaos @flash1293 @maryia-lapata