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

[Uptime] Show URL and metrics on sidebar and waterfall item tooltips #99985

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented May 12, 2021

Summary

Resolves #87272.
Depends on #100147.

Note to reviewers: do not review until #100147 is merged

Adds the URL to the tooltip showing metrics on the waterfall view.

Waterfall Tooltip

Before

image

After

Light mode

image

Dark mode

image

This PR additionally adds the metrics to the tooltip that appears over the sidebar URLs:

Sidebar Tooltip

Before

image

After

Light mode

image

Dark mode

image

Author Checklist

  • Accessibility has been considered, relevant aria tags and other accessibility features implemented
    note: I had put an accessibility label in for the URL, but decided against it since there are no interactive elements in the changes and the text is self-explanatory.
  • Telemetry has been added where relevant
  • Docs have been added to this PR covering any new, changed, or removed features
  • Testing for expected behavior is in place
    • Automated tests exist to cover expected and edge case conditions
    • User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
    • Any special/edge cases that need to be manually tested must be documented
    • Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

Reviewer Checklist

  • Accessibility has been considered, relevant aria tags and other accessibility features implemented
  • Telemetry has been added where relevant
  • Docs have been added to this PR covering any new, changed, or removed features
  • Testing for expected behavior is in place
    • Automated tests exist to cover expected and edge case conditions
    • User acceptance testing has been carried out to ensure the feature functions as expected within the context of how it will be used
    • Any special/edge cases that need to be manually tested must be documented
    • Ensure the new layout works responsively (including down to small phone widths, where makes sense for the user flow, e.g. the error page when reacting to an alert)

For maintainers

@justinkambic justinkambic added release_note:enhancement v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels May 12, 2021
@justinkambic justinkambic self-assigned this May 12, 2021
@justinkambic justinkambic requested a review from a team as a code owner May 12, 2021 19:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@dominiqueclarke
Copy link
Contributor

Since this is a feature, I think we should start using our PR checklist.

Copy link
Contributor

@dominiqueclarke dominiqueclarke left a comment

Choose a reason for hiding this comment

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

I believe the original ticket wanted the metrics tooltip (now with the inclusion of the url), to show when hovering over either the url or the chart element.

From the original ticket

This tooltip that shows the metrics and full URL should be shown when hovering over the URL, and/or the graph bar that is representing the metrics.

@justinkambic justinkambic force-pushed the feature/87272-show-url-on-waterfall-tooltip branch from 8ac94a0 to a99a6b4 Compare May 13, 2021 18:44
@justinkambic
Copy link
Contributor Author

I believe the original ticket wanted the metrics tooltip

Yeah sorry about that, I should've marked this as a draft to indicate I planned on revisiting it before review.

@justinkambic justinkambic changed the title [Uptime] Add URL to waterfall metrics tooltip [Uptime] Show URL and metrics on sidebar and waterfall item tooltips May 14, 2021
@justinkambic justinkambic force-pushed the feature/87272-show-url-on-waterfall-tooltip branch from d82ac96 to 8068749 Compare May 14, 2021 14:49
@paulb-elastic
Copy link
Contributor

@justinkambic I took an early look at this, thanks for adding the metrics to the URL tooltip.

Is the request number (at the beginning of the URL) possible to also be shown with the URL in the right (graph) tooltip, like it is on the left (URL) tooltip?

image

Also, can the left (URL) tooltip be wider to prevent wrapping, more like the right (graph) tooltip:

image

@justinkambic
Copy link
Contributor Author

Is the request number (at the beginning of the URL) possible to also be shown with the URL in the right (graph) tooltip, like it is on the left (URL) tooltip?

Yes, this can be done.

Also, can the left (URL) tooltip be wider to prevent wrapping, more like the right (graph) tooltip:

#100147 should fix this already.

@justinkambic justinkambic force-pushed the feature/87272-show-url-on-waterfall-tooltip branch from 23d0282 to 822411c Compare May 14, 2021 21:49
@justinkambic
Copy link
Contributor Author

justinkambic commented May 14, 2021

@paulb-elastic @dominiqueclarke you can give this one another look now.

@shahzad31
Copy link
Contributor

i think we should align styling with elastic/charts tooltip styling, the way it displays tooltip title
image

another thing, i noticed showing color is different in our tooltip, compared with charts tooltip
image

perhaps we can get rid of our custom tooltip implementation and try to find a way to use elastic chart tooltip? it could be done easily on chart part, but not sure how that will work on side bar url list.

@justinkambic
Copy link
Contributor Author

i think we should align styling with elastic/charts tooltip styling, the way it displays tooltip title

I think we could do this as part of this patch.

another thing, i noticed showing color is different in our tooltip, compared with charts tooltip

The tooltips in the waterfall chart today are uniform in this regard though, no?

perhaps we can get rid of our custom tooltip implementation and try to find a way to use elastic chart tooltip? it could be done easily on chart part, but not sure how that will work on side bar url list.

I'd prefer to use a tooltip that we don't need to maintain as well. Can we do that as a follow-up to this change, rather than creeping the scope further?

@shahzad31
Copy link
Contributor

shahzad31 commented May 18, 2021

@justinkambic definitely i am fine with a follow up.

@justinkambic justinkambic removed the WIP Work in progress label Jun 1, 2021
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

I think it looks great , but i find the experience jarring between viewing chart tooltip and url side bar tooltip.

Delay is too long for sidebar. We should keep it same between chart and sidebar. Either increase both or reduce both.

@justinkambic
Copy link
Contributor Author

Delay is too long for sidebar.

I agree, I noted it in my previous PR that introduced the delay. @formgeist @liciavale can you give this change a look? We'd like to get rid of the delay. I think in a UI such as this users expect that tooltips will appear quickly, esp. if they're just interested in quickly learning the resource name.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

I didn't realize delay wasnt introduced in this PR.

happy to resolve that separately

@justinkambic
Copy link
Contributor Author

happy to resolve that separately

@shahzad31 want to make an issue about unifying these two tooltips, or resolving the issue some other way?

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uptime 613 614 +1

Async chunks

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

id before after diff
uptime 1.1MB 1.1MB +2.0KB

History

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

cc @justinkambic

@justinkambic justinkambic merged commit 8f83090 into elastic:master Jun 4, 2021
@justinkambic justinkambic deleted the feature/87272-show-url-on-waterfall-tooltip branch June 4, 2021 12:48
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 4, 2021
…lastic#99985)

* Add URL to metrics tooltip.

* Add screenreader label for URL container.

* Add metrics to URL sidebar tooltip.

* Rename vars.

* Delete unnecessary code.

* Undo rename.

* Extract component to dedicated file, add tests.

* Fix error in test.

* Add offset index to heading of waterfall chart tooltip.

* Format the waterfall tool tip header.

* Add horizontal rule and bold text for waterfall tooltip.

* Extract inline helper function to module-level for reuse.

* Reuse waterfall tooltip style.

* Style reusable tooltip content.

* Adapt existing chart tooltip to use tooltip content component for better consistency.

* Delete test code.

* Style EUI tooltip arrow.

* Revert whitespace change.

* Delete obsolete test.

* Implement and use common tooltip heading formatter function.

* Add tests for new formatter function.

* Fix a typo.

* Add a comment explaining a style hack.

* Add optional chaining to avoid breaking a test.

* Revert previous change, use RTL wrapper, rename describe block.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 4, 2021
…99985) (#101383)

* Add URL to metrics tooltip.

* Add screenreader label for URL container.

* Add metrics to URL sidebar tooltip.

* Rename vars.

* Delete unnecessary code.

* Undo rename.

* Extract component to dedicated file, add tests.

* Fix error in test.

* Add offset index to heading of waterfall chart tooltip.

* Format the waterfall tool tip header.

* Add horizontal rule and bold text for waterfall tooltip.

* Extract inline helper function to module-level for reuse.

* Reuse waterfall tooltip style.

* Style reusable tooltip content.

* Adapt existing chart tooltip to use tooltip content component for better consistency.

* Delete test code.

* Style EUI tooltip arrow.

* Revert whitespace change.

* Delete obsolete test.

* Implement and use common tooltip heading formatter function.

* Add tests for new formatter function.

* Fix a typo.

* Add a comment explaining a style hack.

* Add optional chaining to avoid breaking a test.

* Revert previous change, use RTL wrapper, rename describe block.

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:enhancement Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Uptime] Show the URL and metrics in a tooltip from the URL and graph bar in the waterfall chart
6 participants