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] New width/delay definition for waterfall sidebar item tooltip #100147

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented May 14, 2021

Summary

Resolves #90909.

Adds a delay of long to the waterfall sidebar tooltip, and specifies as max-width of slightly less than the EUI Breakpoints.

The max-width style is applied to both the EuiToolTip used for the waterfall's sidebar, as well as the custom tooltip passed to EUI Charts for the waterfall chart.

Before

image

After

image

image

image

image

Author 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)

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 14, 2021
@justinkambic justinkambic self-assigned this May 14, 2021
@justinkambic justinkambic requested a review from a team as a code owner May 14, 2021 17:36
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic
Copy link
Contributor Author

My personal opinion is the delay="long" is too long of a wait for the tooltip to appear.

I keep hovering and expecting it to happen much sooner. Perhaps we could add a medium option to the EUI component?

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@justinkambic justinkambic force-pushed the feature/90909-fixed-width-waterfall-url-label branch from 3105360 to 4a79d37 Compare May 26, 2021 16:02
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.

The tooltip position seems to be a bit off. The carrot doesn't seem to land on the proper item.
Screen Shot 2021-05-26 at 12 51 19 PM

@@ -143,7 +143,11 @@ export const WaterfallChartLegendContainer = euiStyled.div`
box-shadow: 0px -1px 4px 0px ${(props) => props.theme.eui.euiColorLightShade};
`; // NOTE: EuiShadowColor is a little too dark to work with the background-color

export const WaterfallChartTooltip = euiStyled.div`
export const WaterfallResponsiveMaxWidth = euiStyled.div`
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this name is a little bit generic. Feels like it may be intended to use in multiple places when this is really just meant to be used for the tooltip. I suppose it could be reused, but I'm not sure as of yet what we're reuse it for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was unsure what to name it, can revise to WaterfallTooltipResponsiveMaxWidth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this name to be tooltip-specific c513c65.

Any better suggestions are welcome 🙂

@justinkambic
Copy link
Contributor Author

The tooltip position seems to be a bit off. The carrot doesn't seem to land on the proper item.

@dominiqueclarke thanks for catching that. I've added a custom margin-top to the tooltip style, and it seems to have improved the layout situation.

May-26-2021 13-49-49

LMK if you have any thoughts on a better way.

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.

LGTM

@@ -80,7 +81,13 @@ export const MiddleTruncatedText = ({ ariaLabel, text, onClick, setButtonRef, ur
<EuiScreenReaderOnly>
<span data-test-subj="middleTruncatedTextSROnly">{text}</span>
</EuiScreenReaderOnly>
<EuiToolTip content={text} position="top" data-test-subj="middleTruncatedTextToolTip">
<WaterfallTooltipResponsiveMaxWidth
as={EuiToolTip}
Copy link
Contributor

Choose a reason for hiding this comment

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

TIL. Does this mean that the div in euiStyled.div is essentially overwritten and the wrapper now becomes EuiToolTip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly that; very cool helper prop for composability/reusability.

@justinkambic
Copy link
Contributor Author

@dominiqueclarke we had another PR edit middle_truncated_text since you've approved the PR. I've added 4db8675 to put the index back in the tooltip content.

I did this because it seems that this is what we want. I'm happy to revert if this has changed. cc @paulb-elastic

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

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
uptime 1.1MB 1.1MB +205.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 390 346 -44
stackAlerts 101 95 -6
total -342

History

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

cc @justinkambic

@paulb-elastic
Copy link
Contributor

@justinkambic thanks, it is better for the index to be included, so that the user maintains context (e.g. knowing it's request # 127 they're seeing in the waterfall chart, and inspecting with the mouseover).

@justinkambic
Copy link
Contributor Author

it is better for the index to be included, so that the user maintains context (e.g. knowing it's request # 127 they're seeing in the waterfall chart, and inspecting with the mouseover).

Duly noted. I think we are ok to merge this then.

@justinkambic justinkambic merged commit f58ddc4 into elastic:master Jun 1, 2021
@justinkambic justinkambic deleted the feature/90909-fixed-width-waterfall-url-label branch June 1, 2021 18:58
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Jun 1, 2021
elastic#100147)

* Add new width definition for waterfall sidebar item. Add delay to tooltip.

* Add default value for style if/when undefined.

* Create shared style for eui breakpoints to use by both waterfall tooltip types.

* Add a comment.

* Use viewport units instead of breakpoints.

* Rename a style.

* Add top margin to prevent tooltip from missing target item. Rename a style.

* Adjust custom `margin-top`.

* Add index to tooltip content.

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 1, 2021
#100147) (#101083)

* Add new width definition for waterfall sidebar item. Add delay to tooltip.

* Add default value for style if/when undefined.

* Create shared style for eui breakpoints to use by both waterfall tooltip types.

* Add a comment.

* Use viewport units instead of breakpoints.

* Rename a style.

* Add top margin to prevent tooltip from missing target item. Rename a style.

* Adjust custom `margin-top`.

* Add index to tooltip content.

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

Co-authored-by: Justin Kambic <justin.kambic@elastic.co>
jloleysens added a commit to jloleysens/kibana that referenced this pull request Jun 2, 2021
…sens/kibana into reporting/new-png-pdf-report-type

* 'reporting/new-png-pdf-report-type' of github.com:jloleysens/kibana: (46 commits)
  [Security Solution] Add Ransomware canary advanced policy option (elastic#101068)
  [Exploratory view] Core web vitals (elastic#100320)
  [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034)
  [Lens] Use a setter function for the dimension panel (elastic#101123)
  [Index Patterns] Fix return saved index pattern object (elastic#101051)
  [CI] For PRs, build TS refs before public api docs check (elastic#100791)
  [Maps] fix line and polygon label regression (elastic#101085)
  Migrate CCR to new ES JS client. (elastic#100131)
  [Canvas] Switch Canvas to use React Router (elastic#100579)
  [Expressions] Use table column ID instead of name when set (elastic#99724)
  [DOCS] Updates docs landing page (elastic#100749)
  [DOCS] Corrects typo in step 3 (elastic#101079)
  [DOCS] Updates runtime example in Discover (elastic#100926)
  Migrate kibana.autocomplete config to data plugin (elastic#100586)
  [Uptime] New width/delay definition for waterfall sidebar item tooltip (elastic#100147)
  [FTR] Use importExport for saved_object/basic archive (elastic#100244)
  [Fleet] Better input for multi text input in agent policy builder (elastic#101020)
  [CI] Buildkite support with Baseline pipeline (elastic#100492)
  [Reporting/Telemetry] Do not send telemetry if we are in screenshot mode (elastic#100388)
  Create API keys with metadata (elastic#100682)
  ...
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
5 participants