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

TimePicker: New time picker dropdown & custom range UI #16811

Merged
merged 51 commits into from
Jun 24, 2019

Conversation

jschill
Copy link
Contributor

@jschill jschill commented Apr 29, 2019

What this PR does / why we need it:
New Time picker in dashboards

Which issue(s) this PR fixes:
Fixes #16805

TODO:

  • Handle UTC
  • Should it be possible to manually edit from/to?
  • Tab between from/to
  • All quick ranges moved from the popover to the select menu
  • Selected quick range should be underlined
  • Add zoom (+ tooltip)
  • UI issues mentioned in this PR

Updated TODO:

  • UTC is not working correctly
  •  Dropdown (SelectButton) menu is not closing when clicking outside
  • Clicking a day cell in calendar should not change time
  • Time picker is not hidden in dashboard settings view

@jschill jschill changed the title feat: Add new picker to DashNavTimeControls Chore: Add new picker to DashNavTimeControls Apr 29, 2019
@jschill jschill force-pushed the 16805-time-picker-dashboards branch from bb35eab to d619324 Compare April 30, 2019 07:08
@torkelo
Copy link
Member

torkelo commented Apr 30, 2019

Nice start on this one!

  • Yes I think manual text input for from/to is needed as you can write custom stuff like from: 1h, to:now+6h
  • I do no think we quick ranges in the Custom popover view, I think they work best as options in the regular dropdown

@jschill jschill force-pushed the 16805-time-picker-dashboards branch 2 times, most recently from 232d805 to f9bd466 Compare May 6, 2019 12:11
@jschill jschill force-pushed the 16805-time-picker-dashboards branch from ff5092f to 7db574b Compare May 7, 2019 15:06
@jschill
Copy link
Contributor Author

jschill commented May 8, 2019

  • I do no think we quick ranges in the Custom popover view, I think they work best as options in the regular dropdown

@torkelo Remind me, was this all quick ranges or just the ones starting with "Last" (like "Last hour"). Do we want to keep the other quick ranges (like "This day last week") in the popup?

Screenshot 2019-05-08 08 07 28

The select scroll will be very long if we move all of them.

@jschill jschill marked this pull request as ready for review May 8, 2019 06:15
@torkelo
Copy link
Member

torkelo commented May 8, 2019

Not sure, can we test with all?

@torkelo
Copy link
Member

torkelo commented May 9, 2019

made a minor design update to fix the popover background. Let me know when you have moved all the quick ranges into the dropdown and this is ready for review & testing.

jschill and others added 7 commits June 8, 2019 09:31
…fana into 16805-time-picker-dashboards

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
* Removed use of Popper not sure we need that here?
* Made active selected item in the list have the "selected" checkmark
* Changed design of popover
* Changed design of and implementation of the Custom selection in the dropdown it did not feel like a item you
could select like the rest now the list is just a normal list
@torkelo torkelo changed the title Chore: Add new picker to DashNavTimeControls TimePicker: New time picker dropdown & custom range UI Jun 9, 2019
Copy link
Contributor

@marefr marefr left a comment

Choose a reason for hiding this comment

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

The time zone refactor looks great! Left some comment to take into consideration.

return { from: fromMoment, to: toMoment, raw: { from: timeOption.from, to: timeOption.to } };
export const mapTimeOptionToTimeRange = (timeOption: TimeOption, timeZone?: TimeZone): TimeRange => {
const fromMoment = stringToDateTimeType(timeOption.from, false, timeZone);
const toMoment = stringToDateTimeType(timeOption.to, true, timeZone);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick, but would be nice to remove any moment references

s/fromMoment/fromDateTime
s/toMoment/toDateTime

isUtc: raw === 'utc',
};
};
export type TimeZone = 'utc' | 'browser';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change this to an enum instead to get safe typings instead of using "magic" strings all over like 'utc' and 'browser'?

Copy link
Member

Choose a reason for hiding this comment

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

This is type safe, any other value will give you error

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but a bit easier to find usage of an enum than strings. So instead of doing timeZone === 'utc' you would do timeZone === TimeZone.UTC`.

Copy link
Member

Choose a reason for hiding this comment

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

true, just this will be huge enum in that case, to support all timezones names and UTC offset variants. maybe constants instead? but that will not be clear that you should use a constant.

Copy link
Member

Choose a reason for hiding this comment

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

maybe this

export type TimeZoneUtc = 'utc';
export type TimeZoneBrowser = 'browser';
export type TimeZone = TimeZoneBrowser | TimeZoneUtc | string;

but there is nothing enforcing use of TimeZoneBrowser etc

Copy link
Contributor

Choose a reason for hiding this comment

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

This is no biggie we can use the current solution for now, just nitpick I guess.

true, just this will be huge enum in that case, to support all timezones names and UTC offset variants.

Using type TimeZone = 'utc' | 'browser'; will also be a huge list!

Probably easier to handle later when implementing support for more time zones. As you discuss we may want to use some sort of TimeZoneOffset field/type later which describes the offset in minutes compared to UTC?

Seems like there are 38 offsets.

packages/grafana-ui/src/utils/datemath.ts Show resolved Hide resolved
from: dateMath.parse(rawRange.from, false, timeZone.raw as any),
to: dateMath.parse(rawRange.to, true, timeZone.raw as any),
from: dateMath.parse(rawRange.from, false, timeZone as any),
to: dateMath.parse(rawRange.to, true, timeZone as any),
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can remove the as any now

from: dateMath.parse(raw.from, false, timeZone.raw as any),
to: dateMath.parse(raw.to, true, timeZone.raw as any),
from: dateMath.parse(raw.from, false, timeZone as any),
to: dateMath.parse(raw.to, true, timeZone as any),
Copy link
Contributor

Choose a reason for hiding this comment

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

Think we can remove the as any now

@torkelo torkelo merged commit 0412a28 into master Jun 24, 2019
@torkelo torkelo deleted the 16805-time-picker-dashboards branch June 24, 2019 12:40
markelog added a commit that referenced this pull request Jun 24, 2019
* master:
  TimePicker: New time picker dropdown & custom range UI (#16811)
  RemoteCache: redis connection string parsing test (#17702)
  Fix link in pkg/README (#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (#15364)
  Settings: Fix typo in defaults.ini (#17707)
  Project: Adds a security policy (#17698)
  Project: Adds support resource docs (#17699)
  Document issue triage process (#17669)
  noImplicitAny: slate (#17681)
  config: fix connstr for remote_cache (#17675)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Jun 24, 2019
…-mapping-to-field

* grafana/master:
  Elasticsearch: Visualize logs in Explore (grafana#17605)
  Grafana-CLI: Wrapper for `grafana-cli` within RPM/DEB packages and config/homepath are now global flags (grafana#17695)
  Add guidelines for SQL date comparisons (grafana#17732)
  Docs: clarified usage of go get and go mod (grafana#17637)
  Project: Issue triage doc improvement (grafana#17709)
  Improvement: Grafana release process minor improvements (grafana#17661)
  TimePicker: New time picker dropdown & custom range UI (grafana#16811)
  RemoteCache: redis connection string parsing test (grafana#17702)
  Fix link in pkg/README (grafana#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (grafana#15364)
  Settings: Fix typo in defaults.ini (grafana#17707)
  Project: Adds a security policy (grafana#17698)
  Project: Adds support resource docs (grafana#17699)
  Document issue triage process (grafana#17669)
  noImplicitAny: slate (grafana#17681)
  config: fix connstr for remote_cache (grafana#17675)
  Explore: Improves performance of Logs element by limiting re-rendering (grafana#17685)
  Docs: Flag serve_from_sub_path as available in 6.3 (grafana#17674)
  @grafana/runtime: expose config and loadPluginCss (grafana#17655)
  noImplicitAny: Fix basic errors (grafana#17668)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Jun 25, 2019
* grafana/master:
  Elasticsearch: Visualize logs in Explore (grafana#17605)
  Grafana-CLI: Wrapper for `grafana-cli` within RPM/DEB packages and config/homepath are now global flags (grafana#17695)
  Add guidelines for SQL date comparisons (grafana#17732)
  Docs: clarified usage of go get and go mod (grafana#17637)
  Project: Issue triage doc improvement (grafana#17709)
  Improvement: Grafana release process minor improvements (grafana#17661)
  TimePicker: New time picker dropdown & custom range UI (grafana#16811)
  RemoteCache: redis connection string parsing test (grafana#17702)
  Fix link in pkg/README (grafana#17714)
  Dashboard: Use Explore's Prometheus editor in dashboard panel edit (grafana#15364)
  Settings: Fix typo in defaults.ini (grafana#17707)
  Project: Adds a security policy (grafana#17698)
  Project: Adds support resource docs (grafana#17699)
  Document issue triage process (grafana#17669)
  noImplicitAny: slate (grafana#17681)
  config: fix connstr for remote_cache (grafana#17675)
  Explore: Improves performance of Logs element by limiting re-rendering (grafana#17685)
  Docs: Flag serve_from_sub_path as available in 6.3 (grafana#17674)
ryantxu added a commit to ryantxu/grafana that referenced this pull request Jun 25, 2019
* grafana/master: (33 commits)
  API: get list of users with additional auth info (grafana#17305)
  TimePicker: fixed minor issues with new timepicker (grafana#17756)
  Explore: Parses and updates TimeSrv in one place in Explore (grafana#17677)
  @grafana/ui: release (grafana#17754)
  Password: Remove PasswordStrength (grafana#17750)
  Devenv:SAML: devenv block with saml test app (grafana#17733)
  LDAP:Docs: add information on LDAP sync feature and update LDAP sync default (grafana#17689)
  Graph: Add data links feature (click on graph) (grafana#17267)
  Explore: Changes LogsContainer from a PureComponent to a Component (grafana#17741)
  Chore: Remove tether and tether drop dependency in grafana/ui (grafana#17745)
  noImplicitAny: time region manager etc. (grafana#17729)
  Panel: Fully escape html in drilldown links (was only sanitized before)  (grafana#17731)
  Alerting: Improve alert rule testing (grafana#16286)
  Elasticsearch: Visualize logs in Explore (grafana#17605)
  Grafana-CLI: Wrapper for `grafana-cli` within RPM/DEB packages and config/homepath are now global flags (grafana#17695)
  Add guidelines for SQL date comparisons (grafana#17732)
  Docs: clarified usage of go get and go mod (grafana#17637)
  Project: Issue triage doc improvement (grafana#17709)
  Improvement: Grafana release process minor improvements (grafana#17661)
  TimePicker: New time picker dropdown & custom range UI (grafana#16811)
  ...
@marefr marefr added this to the 6.3.0-beta1 milestone Aug 6, 2019
@ying-jeanne ying-jeanne added the pr/external This PR is from external contributor label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The new time picker in the dashboards
4 participants