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

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

    return [yesterdayOption, aWeekAgoOption];
  }

  // 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

@@ -23,6 +23,8 @@ const PrependContainer = styled.div`
  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 as a code owner February 2, 2021 14:40

botelastic bot added the Team:APM All issues that need APM UI Team support 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cauemarcondes commented Feb 2, 2021

elasticmachine commented Feb 2, 2021

sorenlouv Feb 2, 2021

cauemarcondes Feb 2, 2021

sorenlouv Feb 2, 2021

cauemarcondes Feb 3, 2021

sorenlouv Feb 2, 2021

dgieselaar Feb 2, 2021

dgieselaar Feb 2, 2021

dgieselaar Feb 2, 2021

sorenlouv Feb 2, 2021

formgeist Feb 3, 2021

sorenlouv Feb 3, 2021

cauemarcondes Feb 3, 2021

dgieselaar Feb 3, 2021

cauemarcondes Feb 3, 2021

formgeist left a comment

cauemarcondes commented Feb 3, 2021

formgeist commented Feb 4, 2021

cauemarcondes commented Feb 4, 2021

sorenlouv commented Feb 4, 2021 •

edited

Loading

cauemarcondes commented Feb 4, 2021

sorenlouv commented Feb 4, 2021 •

edited

Loading

sorenlouv commented Feb 4, 2021 •

edited

Loading

cauemarcondes commented Feb 4, 2021

formgeist commented Feb 4, 2021

cauemarcondes commented Feb 4, 2021

kibanamachine commented Feb 4, 2021

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

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

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

Conversation

cauemarcondes commented Feb 2, 2021

elasticmachine commented Feb 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

formgeist left a comment

Choose a reason for hiding this comment

cauemarcondes commented Feb 3, 2021

formgeist commented Feb 4, 2021

cauemarcondes commented Feb 4, 2021

sorenlouv commented Feb 4, 2021 • edited Loading

cauemarcondes commented Feb 4, 2021

sorenlouv commented Feb 4, 2021 • edited Loading

sorenlouv commented Feb 4, 2021 • edited Loading

cauemarcondes commented Feb 4, 2021

formgeist commented Feb 4, 2021

cauemarcondes commented Feb 4, 2021

kibanamachine commented Feb 4, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

sorenlouv commented Feb 4, 2021 •

edited

Loading

sorenlouv commented Feb 4, 2021 •

edited

Loading

sorenlouv commented Feb 4, 2021 •

edited

Loading