[APM] Enabling yesterday option when 24 hours is selected by cauemarcondes · Pull Request #90017 · elastic/kibana

cauemarcondes · 2021-02-02T14:40:29Z

elasticmachine · 2021-02-02T14:40:34Z

Pinging @elastic/apm-ui (Team:apm)

sorenlouv · 2021-02-02T16:35:33Z

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx


  // Less than one week
-  if (dateDiff <= 7) {
+  if (dateDiffInHours <= weekDurationInHours) {


Do we have a test for this component?

https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/public/components/shared/time_comparison/index.test.tsx

Great! I'm still wondering why the test doesn't need updating when the implementation changes.

I added a few more tests, also to cover what was discussed in the comments below.

sorenlouv · 2021-02-02T18:48:56Z

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx

  padding: 0 ${px(unit)};
 `;

+const weekDurationInHours = moment.duration(7, 'days').asHours();


nit: There's always the vanilla option. Might also be the simplest to read:

Suggested change

const weekDurationInHours = moment.duration(7, 'days').asHours();

const weekDurationInHours = 24 * 7

IMHO we should consider using moment here, but scoped to the current time, to deal with issues like DST. E.g.:

const aDayAgo = momentEnd.subtract(1, 'days').getTime(); const isAtLeastADayAgo = momentStart.getTime() <= aDayAgo;

const aWeekAgo = momentEnd.subtract(1, 'week').getTime(); const isAtLeastAWeekAgo = momentStart.getTime() <= aWeekAgo;

(we should figure out how exactly moment.subtract() deals with DST ofc)

I'll admit I'm not quite sure why we are using hours here, are we rounding?

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Good point. The current copy is only applicable if the end range is now.
I think we could keep the labels if we add a check like:

const = end === Date.now(); if (dateDiffInHours <= 24 && isEndNow) {

We might have to add some padding like:

const isEndNow = end + 1000 >= Date.now(); if (dateDiffInHours <= 24 && isEndNow) { return [yesterdayOption, aWeekAgoOption]; }

If isEndNow is false we'll fall back to prevPeriodOption which might be ok.

Btw, should this not be "a day before" and "a week before" (the current time range), rather than yesterday/a week ago?

Good catch! And I agree, the more clear we can be with the relative time the easier it is to parse for the user. So if we can add a check as @sqren proposes, I'm very happy 👍

End is rounded.

That's why I thought we could add padding. But since rounding is up to 5 minutes we'd need a lot of padding so I agree, not a great solution.

But maybe we can use the query parameter? E.g. if rangeTo=='now'

Much better idea. Let's try to do that.

I'll admit I'm not quite sure why we are using hours here, are we rounding?

@dgieselaar I switched to use hours because when I used days and the difference between the dates is for example 25 hours the days difference still returns 1. But I only want to show the yesterday option when the difference is less or equal to 24 hours.

you can pass a third argument, a boolean, to diff to get a floating point number instead of an integer. then you could just use <= 1. https://jsfiddle.net/8whg49qj/

thanks will do it

formgeist

LGTM from a UX design 👁️

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx

cauemarcondes · 2021-02-03T17:36:50Z

@formgeist I added the time when showing the previous period, I think it'll help the user to understand what is the exact time that is being compared.

Let me know if you don't want to keep it.

formgeist · 2021-02-04T07:29:01Z

@formgeist I added the time when showing the previous period, I think it'll help the user to understand what is the exact time that is being compared.

Let me know if you don't want to keep it.

No, this is good 👍

cauemarcondes · 2021-02-04T08:37:49Z

@elasticmachine merge upstream

sorenlouv · 2021-02-04T12:32:20Z

When comparison is enabled but there is only one option we show that option as disabled. It looks to me like the compared date (03/02 14:00 -...) is not enabled, when actually we just want to tell the user that there is only one option (not that it's disabled).

In contrast: when comparison actually is disabled we don't disable the selector. Why is this? Looks to me like we compare with Yesterday.

Suggestion
When comparison is disabled, the dropdown should also be disabled.
When comparison is enabled, the dropdown should not be disabled. If there is only one option we could consider replacing the dropdown with just a label (so it's clear that it cannot be changed). But I don't think that's super important.

cauemarcondes · 2021-02-04T12:50:56Z

When comparison is disabled, the dropdown should also be disabled.

Will disable the select box too.

If there is only one option we could consider replacing the dropdown with just a label (so it's clear that it cannot be changed). But I don't think that's super important.

This was a discussion a had with @formgeist a few days ago, the problem is that select input doesn't have a read-only state. EUI would have to create a fake "read-only" state. There's an issue open for this elastic/eui#3510 (comment).

But in the end, @formgeist and I agreed that we could go like this (disabling the select) for now, and we could change to a label or maybe a text input with read-only property later if needed.

sorenlouv · 2021-02-04T13:08:41Z

we could go like this (disabling the select) for now, and we could change to a label or maybe a text input with read-only property later if needed.

What about just leaving it as non disabled until we have a read-only property? Afaik we don't disable other dropdowns when there's only one option, do we? Eg. transaction type selector or agent config.

sorenlouv · 2021-02-04T13:11:08Z

Btw. I don't think read-only is the right approach either tbh. A drop-down is always read only (in contrast to an input field). If we want to make it clear that there's only a single option we shouldn't use a dropdown but a label. But I'm not sure it's critical to indicate this, is it?

cauemarcondes · 2021-02-04T14:16:44Z

What about just leaving it as non disabled until we have a read-only property?

That works for me too.

formgeist · 2021-02-04T14:20:14Z

What about just leaving it as non disabled until we have a read-only property?

That works for me too.

Sounds good to me too 👍

…arison-ui

cauemarcondes · 2021-02-04T14:36:50Z

kibanamachine · 2021-02-04T16:16:46Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b3362b4

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`apm`	5.2MB	5.2MB	+769.0B

History

💚 Build #103727 succeeded 826595e71efa22a24fffc424ff4da73449a264c4
💚 Build #103507 succeeded 7619127
💚 Build #103004 succeeded 14d8cd4

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

) * enabling yesterday option when 24 hours is selected * addressing PR comments * addressing PR comments * enabling select box

* master: (244 commits) [maps] Top hits per entity--change to title to use recent, minor edits (elastic#89254) [DOCS] Update installation details (elastic#90354) RFC for automatically generated typescript API documentation for every plugins public services, types, and functionality (elastic#86704) Elastic Maps Server config is `host` not `hostname` (elastic#90234) Use doc link services in index pattern management (elastic#89937) [Fleet] Managed Agent Policy (elastic#88688) [Workplace Search] Fix Source Settings bug (elastic#90242) [Enterprise Search] Refactor MockRouter test helper to not store payload (elastic#90206) Use doc link service in more Stack Monitoring pages (elastic#89050) [App Search] Relevance Tuning logic - actions and selectors only, no listeners (elastic#89313) Remove UI filters from UI (elastic#89793) Use newfeed.service config for all newsfeeds (elastic#90252) skip flaky suite (elastic#85086) Add readme to geo containment alert covering test alert setup (elastic#89625) [APM] Enabling yesterday option when 24 hours is selected (elastic#90017) Test user for maps tests under import geoJSON tests (elastic#86015) [Lens] Hide column in table (elastic#88680) [Security Solution][Detections] Reduce detection engine reliance on _source (elastic#89371) [Discover] Minor cleanup (elastic#90260) [Search Session][Management] Rename "cancel" button and delete "Reload" button (elastic#90015) ...

…90295) * enabling yesterday option when 24 hours is selected * addressing PR comments * addressing PR comments * enabling select box

enabling yesterday option when 24 hours is selected

14d8cd4

cauemarcondes added release_note:skip Skip the PR/issue when compiling release notes v7.12.0 apm:comparison labels Feb 2, 2021

cauemarcondes requested a review from a team February 2, 2021 14:40

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Feb 2, 2021

cauemarcondes requested a review from formgeist February 2, 2021 14:40

sorenlouv approved these changes Feb 2, 2021

View reviewed changes

sorenlouv reviewed Feb 2, 2021

View reviewed changes

formgeist approved these changes Feb 2, 2021

View reviewed changes

sorenlouv reviewed Feb 2, 2021

View reviewed changes

x-pack/plugins/apm/public/components/shared/time_comparison/index.tsx Show resolved Hide resolved

cauemarcondes changed the title ~~enabling yesterday option when 24 hours is selected~~ [APM] Enabling yesterday option when 24 hours is selected Feb 3, 2021

addressing PR comments

7619127

addressing PR comments

f80a99d

cauemarcondes requested a review from dgieselaar February 4, 2021 10:34

cauemarcondes added 2 commits February 4, 2021 15:25

Merge branch 'master' of github.com:elastic/kibana into apm-time-comp…

ed89e4b

…arison-ui

enabling select box

b3362b4

cauemarcondes force-pushed the apm-time-comparison-ui branch from 826595e to b3362b4 Compare February 4, 2021 14:36

cauemarcondes requested a review from sorenlouv February 4, 2021 14:37

cauemarcondes merged commit 7a42a6f into elastic:master Feb 4, 2021

cauemarcondes deleted the apm-time-comparison-ui branch February 4, 2021 16:20

cauemarcondes mentioned this pull request Feb 4, 2021

[7.x] [APM] Enabling yesterday option when 24 hours is selected (#90017) #90295

Merged

cauemarcondes added a commit that referenced this pull request Feb 5, 2021

[APM] Enabling yesterday option when 24 hours is selected (#90017) (#…

e5a25f9

…90295) * enabling yesterday option when 24 hours is selected * addressing PR comments * addressing PR comments * enabling select box

	const weekDurationInHours = moment.duration(7, 'days').asHours();
	const weekDurationInHours = 24 * 7

Conversation

cauemarcondes commented Feb 2, 2021

Uh oh!

elasticmachine commented Feb 2, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

formgeist left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cauemarcondes commented Feb 3, 2021

Uh oh!

formgeist commented Feb 4, 2021

Uh oh!

cauemarcondes commented Feb 4, 2021

Uh oh!

sorenlouv commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cauemarcondes commented Feb 4, 2021

Uh oh!

sorenlouv commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sorenlouv commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cauemarcondes commented Feb 4, 2021

Uh oh!

formgeist commented Feb 4, 2021

Uh oh!

cauemarcondes commented Feb 4, 2021

Uh oh!

kibanamachine commented Feb 4, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sorenlouv commented Feb 4, 2021 •

edited

Loading

sorenlouv commented Feb 4, 2021 •

edited

Loading

sorenlouv commented Feb 4, 2021 •

edited

Loading