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(unify): launchpad header breadcrumbs and reusable tooltip component #20648

Merged

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Mar 16, 2022

User facing changelog

  • have a new, last breadcrumb for e2e vs ct
  • project name should link back to testing type selection screen
  • display branch
  • branch is truncated if needed, and has a tooltip
  • made a reusable tooltip component and replaced our old ones with it in several places

Screen Shot 2022-04-12 at 10 15 32 AM

Screen Shot 2022-04-12 at 10 15 46 AM

Additional details

How has the user experience changed?

PR Tasks

  • Have tests been added/updated?
  • Has the original issue (or this PR, if no issue exists) been tagged with a release in ZenHub? (user-facing changes only)
  • Has a PR for user-facing changes been opened in cypress-documentation?
  • Have API changes been updated in the type definitions?
  • Have new configuration options been added to the cypress.schema.json?

@cypress
Copy link

cypress bot commented Mar 16, 2022



Test summary

17872 0 217 0Flakiness 4


Run details

Project cypress
Status Passed
Commit 18cf600
Started Apr 19, 2022 2:58 PM
Ended Apr 19, 2022 3:11 PM
Duration 13:44 💡
OS Linux Debian - 10.10
Browser Multiple

View run in Cypress Dashboard ➡️


Flakiness

commands/net_stubbing.cy.ts Flakiness
1 network stubbing > intercepting request > can delay and throttle a StaticResponse
commands/xhr.cy.js Flakiness
1 ... > no status when request isnt forced 404
cypress/proxy-logging.cy.ts Flakiness
1 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second
2 Proxy Logging > request logging > xhr log has response body/status code when xhr response is logged second

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

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Mar 16, 2022

Thanks for taking the time to open a PR!

@mapsandapps mapsandapps marked this pull request as ready for review March 17, 2022 16:43
Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

Looks good overall, a couple of changes requested around accessibility and guarding against clearing the testing type while config is loading.

It's also a good time to confirm with extra dynamic content from users in the header now, how comfortable are we with this wrapping that we see here? That might not need to be addressed in this PR, though, until we have a general direction about wrapping vs truncating.

Screen Shot 2022-03-18 at 7 34 48 AM

@@ -28,7 +31,28 @@
v-if="props.gql?.currentProject"
class="h-16px mr-2px min-w-16px icon-dark-gray-200"
/>
<span class="text-body-gray-700">{{ props.gql?.currentProject?.title }}</span>
<span
Copy link
Contributor

Choose a reason for hiding this comment

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

For accessibility reasons, we should make this either a link or a button so it can receive keyboard focus and be announced the right way. Before it becomes interactive, the disabled link pattern here seems like it would work:

<a role="link" aria-disabled="true">project-name</a>

I think we should follow this pattern for the Projects link as well, we are already close to it there - though that doesn't need to get updated in this PR.

The "link with a fake href because it actually uses a click handler to trigger a mutation that updates state that will move the UI to a new 'page'" doesn't seem ideal at first, but it may be the clearest pattern to understand from a user perspective, in terms of how these breadcrumbs behave.

Copy link
Contributor Author

@mapsandapps mapsandapps Mar 18, 2022

Choose a reason for hiding this comment

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

I didn't think you should ever have an a without an href, so this was news to me!

In my update, I went ahead and used <nav> and <ol> for this, which i had been intending to do if i made a reusable breadcrumb component. LMK what you think about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, I think that's good. I had only found the disabled link pattern recently, I think we needed it somewhere else in launchpad as well.

@mapsandapps
Copy link
Contributor Author

I'm gonna put this back into draft and try to see what we should do about the long branch name and/or project name UI bug.

@mapsandapps mapsandapps marked this pull request as draft March 21, 2022 17:09
@mapsandapps mapsandapps marked this pull request as ready for review April 11, 2022 21:55
@emilyrohrbough
Copy link
Member

emilyrohrbough commented Apr 12, 2022

@mapsandapps Any update on this?

EDIT: nevermind. I just read it was marked for "ready for review". I scanned to quickly! Are you able to update the summary before/after with your recent changes?

@mapsandapps mapsandapps changed the title feat(unify): launchpad header breadcrumbs feat(unify): launchpad header breadcrumbs and reusable tooltip component Apr 12, 2022
@mapsandapps
Copy link
Contributor Author

@emilyrohrbough oops, good call! i've updated the description and the title as well

@marktnoonan
Copy link
Contributor

marktnoonan commented Apr 13, 2022

I had some trouble getting the tooltip to dismiss: https://www.loom.com/share/7ee0e62619ce45f98ae4e5a47b7ee6cc - this seems to only be a problem in launchpad, the main app nav versions are perfect 🤔 .

I don't see the selector playground ones:
Screen Shot 2022-04-13 at 4 45 38 PM

And hmm, I was able to get some of these to not disappear properly in the runner.

Screen Shot 2022-04-13 at 4 46 50 PM

I know I'm the one who recommended the floating-vue library, wonder why it is being weird.

@mapsandapps
Copy link
Contributor Author

@marktnoonan I updated the persistence behavior. LMK if it looks good to you now. As for the selector playground ones, those are actually less tooltips and more (oddly placed) toasts. From what I saw, they should appear on click, not on hover.

@marktnoonan
Copy link
Contributor

Thanks for clarifying @mapsandapps, sorry to be slow responding here. You're right about the current implementation of the tooltips, 9.x was different and while I think we want that experience in 10.0, it's not at all in scope for this PR, so I'll make a new ticket to track it.

Going to pull this down and review properly now.

tooltips.in.the.playground.mov

Copy link
Contributor

@marktnoonan marktnoonan left a comment

Choose a reason for hiding this comment

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

These changes work, and I'm delighted we can copy text from these tooltips! I think we should merge this PR as is (once conflicts are resolved), though I see some accessibility issues to address when we are back in the tooltips (which is really soon, and making a perfect tooltip for all cases was not the goal of this PR). The element you hover over should show the tooltip when you focus with the keyboard. But only in "focus-visible" type situations, not if focus lands on an element by clicking.

One other minor thing I noticed the "link" to the project breadcrumb can be clicked even when the browser is already open. It works fine, just seems a little less clear than the "switch testing type" button that does the same thing. I feel like there's just a little tiny window where that breadcrumb being a link is functional, but that's a separate topic.

I'll hit that ✔️ when the conflicts are resolved and this is ready to merge.

@lmiller1990
Copy link
Contributor

Nice, looks great 👍

Should we hit approve on Percy?

@mapsandapps mapsandapps merged commit 43289e7 into 10.0-release Apr 19, 2022
@mapsandapps mapsandapps deleted the mapsandapps/feat/launchpad-header-breadcrumbs branch April 19, 2022 15:33
tgriesser added a commit that referenced this pull request Apr 20, 2022
…e-config

* 10.0-release:
  chore: Move component-index generation to scaffold-config package (#21090)
  fix: label text should be clickable to toggle snapshot highlight (#21122)
  feat: add next preset to webpack-dev-server-fresh (#21069)
  chore: add dev-servers as deps to server to be included in the binary (#21142)
  fix: do not highlight preExtension if selected option is renameFolder (#21121)
  fix(unify): Remove 'Reconfigure' dropdown from Testing Type chooser (#21063)
  feat(unify): launchpad header breadcrumbs and reusable tooltip component (#20648)
  test: add windows-test-kitchensink job (#20847)
  fix: aut centering and height calculation (#21019)
  chore: fix tests that fail on windows (#21055)
  fix: running a new test after already having run tests (#21087)
  fix: ct project setup framework keys for next and nuxt (#21116)
  fix: remove MountReturn from scaffolded ct support file (#21119)
  fix: do not scaffold fixtures if folder exist (#21078)
  fix: revert "fix: types for Cypress.Commands.add (#20376)" (#21104)
  chore: Update Chrome (stable) to 100.0.4896.127 and Chrome (beta) to 101.0.4951.34 (#21083)
  fix: types for Cypress.Commands.add (#20376) (#20377)
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

4 participants