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

feat: IATR-M0 Page Header (#24722) #25360

Merged
merged 4 commits into from
Jan 6, 2023

Conversation

marktnoonan
Copy link
Contributor

@marktnoonan marktnoonan commented Jan 4, 2023

User facing changelog

NA, merging to feature branch

Additional details

Added an optional originalTitle property to the MappedTitlePart type. There are numerous other ways to handle this but this one seems fine to me. We could maybe work on the readability of this component at a future time.

Steps to test

Visit the component test for DebugFailedTest.vue and let it run. Observe tooltip appears when you hover over the ellipses:

Screen Shot 2023-01-04 at 1 05 17 PM

Sections that are truncated by ellipses, but still visible, will not have this tooltip, based on the wording of the original issue. To do that we would need a little more sophisticated approached to determine that the browser has truncated the text and conditionally enable the tooltip, the presence of the class alone is not enough. So that is out of scope for this ticket.

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • [na] Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • [na] Has a PR for user-facing changes been opened in cypress-documentation?
  • [na] Have API changes been updated in the type definitions?

@marktnoonan marktnoonan requested review from a team and warrensplayer January 4, 2023 18:10
@cypress
Copy link

cypress bot commented Jan 4, 2023



Test summary

26492 0 1180 0Flakiness 31


Run details

Project cypress
Status Passed
Commit 59a78e3
Started Jan 6, 2023 1:37 PM
Ended Jan 6, 2023 1:55 PM
Duration 18:24 💡
OS Linux Debian -
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

create-from-component.cy.ts Flakiness
1 ... > runs generated spec
2 ... > runs generated spec
commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
2 network stubbing > intercepting request > can delay and throttle a StaticResponse
3 network stubbing > intercepting request > can delay and throttle a StaticResponse
This comment includes only the first 5 flaky tests. See all 31 flaky tests in the Cypress Dashboard.

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

@mike-plummer mike-plummer requested a review from a team January 5, 2023 16:36
Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

Need to account for multiple "MIDDLE" parts being hidden by the ellipsis

@@ -197,6 +197,9 @@ describe('<DebugFailedTest/>', () => {

assertRowContents(testResult)

cy.contains('...').realHover()
cy.contains('[data-cy=tooltip-content]', 'Test content 2').should('be.visible')
Copy link
Contributor

Choose a reason for hiding this comment

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

This should contain everything that was hidden by the ellipsis separated by >

Suggested change
cy.contains('[data-cy=tooltip-content]', 'Test content 2').should('be.visible')
cy.contains('[data-cy=tooltip-content]', 'Test content 2 > Test content 3 > Test content 4').should('be.visible')

@@ -30,7 +30,17 @@
titlePart.type === 'LAST-1' ? 'shrink-0 whitespace-pre' :
titlePart.type === 'LAST-0' ? 'pl-2.5 truncate' : 'px-2.5 truncate'"
>
{{ titlePart.title }}
<template v-if="titlePart.title === '...'">
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<template v-if="titlePart.title === '...'">
<template v-if="titlePart.type === 'ELLIPSIS'">

Check with type instead of title

@@ -121,6 +133,7 @@ const failedTestData = computed(() => {
{
title: '...',
type: 'ELLIPSIS',
originalTitle: ele,
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to gather up all the "MIDDLE" parts to construct the originalTitle passed to the ELLIPSIS tooltip

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks! I think I was missing the underlying concept here, that it's really combining the parts 👍

Copy link
Contributor

@warrensplayer warrensplayer left a comment

Choose a reason for hiding this comment

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

Updates look great. I really like the refactor for some of the enum names and variable names

@warrensplayer warrensplayer merged commit c33ba30 into feature/IATR-M0 Jan 6, 2023
@warrensplayer warrensplayer deleted the marktnoonan/iatr-tooltip-ellipsis branch January 6, 2023 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants