fix: Spec List: Added tooltips to git icons created/modified #21536

MuazOthman · 2022-05-17T15:05:24Z

Closes CLOUD-576

User facing changelog

On Spec list page, show tooltips for git status icons when created/modified for clarity
On Spec list page, address the case when no git information is available to show no icons and file update time from file system

Additional details

How has the user experience changed?

PR Tasks

[na] 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)
[na] Has a PR for user-facing changes been opened in cypress-documentation?
[na] Have API changes been updated in the type definitions?
[na] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2022-05-17T15:05:26Z

Thanks for taking the time to open a PR!

Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.
Become familiar with the Code Review Checklist for guidelines on coding standards and what needs to be done before a PR can be merged.

cypress · 2022-05-17T15:31:35Z

Test summary

37537 • 0 • 454 • 0 • 0

Run details


Project	cypress
Status	Passed
Commit	`d6df106`
Started	May 26, 2022 8:26 PM
Ended	May 26, 2022 8:47 PM
Duration	20:19 💡
OS	Linux Debian - 10.11
Browser	Multiple

View run in 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

marktnoonan · 2022-05-17T18:27:45Z

The description says "na" for "Have tests been added/updated?", but this seems like some new behavior that should be covered in a test. Are there existing tests that already cover these tooltips?

ZachJW34 · 2022-05-17T20:14:59Z

I'm seeing some weird behavior where the tooltip won't show up after the icon changes. Not sure if it is this PR that introduces this or not

Screen.Recording.2022-05-17.at.3.12.39.PM.mov

packages/app/src/specs/SpecListGitInfo.vue

emilyrohrbough · 2022-05-18T15:09:36Z

packages/app/src/specs/SpecListGitInfo.vue

    >
      <component
        :is="classes.icon"
        :class="classes.iconClasses"
      />
      <template
-        v-if="classes.showTooltip"
        #popper
      >
        <div>
          <p class="max-w-sm text-sm truncate overflow-hidden">


Can we conditionally render this if there is a tooltip message to show?

There will always be a tooltip to show. Prior to this PR, we had cases when we conditionally render tooltips, but now a tooltip should show in all cases but contents can vary.

Since the default was null I assumed there was cases when there wasn't content.

Is there always a tooltip even when the SCM is not available and/or git is not used in the project?

If there's no SCM data available, this whole component won't be rendered

cypress/packages/app/src/specs/SpecsList.vue

Line 105 in 06a0283

v-if="row.data.isLeaf && row.data.data?.gitInfo"

emilyrohrbough · 2022-05-18T15:09:55Z

packages/app/src/specs/SpecListGitInfo.vue

    >
      <component
        :is="classes.icon"
        :class="classes.iconClasses"
      />
      <template
-        v-if="classes.showTooltip"


Can we conditionally render this if there is tooltip text to show?

There will always be a tooltip to show. Prior to this PR, we had cases when we conditionally render tooltips, but now a tooltip should show in all cases but contents can vary.

emilyrohrbough · 2022-05-18T15:10:59Z

packages/app/src/specs/SpecListGitInfo.vue

-const gitTooltipSubtext = computed(() => {
+const tooltipMainText = computed(() => {
+  switch (props.gql?.statusType) {
+    case 'unmodified': return props.gql?.subject


what does this props.gql?.subject show?

This is the commit subject. I think #21139 can further show previous work updated in this PR.

MuazOthman · 2022-05-18T15:48:09Z

@marktnoonan thanks for the feedback. I'll review the tests.
@ZachJW34 thanks for catching that issue. I'll look into it.

ZachJW34 · 2022-05-18T22:58:54Z

@MuazOthman if this is an existing issue, I'm cool with getting this merged and following up with an issue for it.

tgriesser · 2022-05-20T17:27:21Z

packages/graphql/schemas/schema.graphql

@@ -760,6 +760,7 @@ type GitInfo {
 enum GitInfoStatusType {
  created
  modified
+  no_git_info


Not seeing the related gql- enum change, was this edited in the SDL file directly?

Also, we generally use camelCase everywhere for variable naming, unless it's CONSTANT_CASE, so noGitInfo

I corrected to follow convention.
The main update is in packages/types/src/git.ts

cypress/packages/types/src/git.ts

Line 1 in b8884b5

export const gitStatusType = ['modified', 'created', 'unmodified', 'noGitInfo'] as const

…6-latest-update-design-updates

MuazOthman · 2022-05-23T14:45:52Z

@ZachJW34 this issue related to tooltips refusing to show up seems odd, would you have time to help look into it tomorrow maybe?
Meanwhile, I guess we can get this PR merged if since it was an existing issue and the updates in this PR need to go into 10.0.

…CLOUD-576-latest-update-design-updates

marktnoonan

@MuazOthman could we please have a bit more description on this PR, it seems to do more than just add tooltips for the icons. Can we give context to the other changes made? It's hard to review without this context. A good example is the new tooltip on the header:

This is not mentioned in the PR or the ticket as far as I can tell, and it looks different to other existing tooltips. What's my reference point to approve this change? I'm sure there is a figma link but don't know where to find it.

There are some other refactors as well. These may be improvements but it's not clear how they relate to what's described in the PR.

The main change request I have is that this PR appears to introduce something that breaks keyboard navigation through the specs list. After tabbing onto an icon that tiggers a tooltip, it gains focus and the next tab stop is the browser address bar:

tab-stops-not-working.mov

I confirmed this doesn't happen on 10.0-release - the icons still show tooltips on focus but I can immediately tab down through the next rows as expected.

On the other hand the column header does not receive keyboard focus and so doesn't reveal its tooltip to keyboard users. Ideally the header and the icons in the rows would behave the same way when focused.

I made a suggestion in the e2e spec for where to add coverage related to the tooltip content changing based on the git status - I think without this coverage we would not catch a regression if these tooltips were were removed/changed.

marktnoonan · 2022-05-24T16:19:06Z

packages/app/cypress/e2e/specs_list_actual_git_repo.cy.ts

-      .should('have.class', 'icon-light-gray-500')
-    }
+    cy.get('[data-cy-row="dom-container.spec.js"] [data-cy="git-info-row"] svg')
+    .should('have.class', 'icon-light-gray-500')


Here we can use cy.trigger('mouseenter') to expand the tooltips and then assert their contents?

marktnoonan · 2022-05-24T16:30:51Z

packages/app/src/specs/LastUpdatedHeader.vue

+            {{ t('specPage.lastUpdated.tooltip.gitInfo') }}
+          </ExternalLink>
+        </i18n-t>
+        <!-- <i18n-t


We can remove this?

marktnoonan · 2022-05-24T17:10:38Z

packages/frontend-shared/src/components/Tooltip.cy.tsx

+    cy.contains('Tooltip 0')
+  })
+
+  it('playground - interactive', { viewportHeight: 400 }, () => {


Since interactive is intended to have a different appearance than regular tooltips, could we add a percy snapshot to this and the other test, and update the test descriptions?

marktnoonan · 2022-05-25T00:06:21Z

packages/frontend-shared/src/components/Tooltip.vue

+    :popper-triggers="['hover']"
+    :hide-triggers="['hover']"
+    :auto-hide="!isInteractive"
+    :theme="theme"
+    :placement="placement ?? 'auto'"


I did a little more testing and found what I think are some other side effects related to these changes that surface when using the keyboard, leading to unexpected states:

After comparing tooltips in general with 10.0-release, it seems something in here is also affecting the tooltips in the Selector Playground - both their positioning and focus behavior, moving focus out to the address bar immediately when a button that has a tooltip is focused. Let me know if you can reproduce, here's what I did:

https://www.loom.com/share/598305649aa741499ca1f909b80e31db - 10.0-release
https://www.loom.com/share/469f94e459554d729c5fe13b63489f2a - this branch

This seem like pretty extreme effects for such a small set of changes, so maybe something else is up with my local env. Though the changed positioning in the Selector Playground is also captured in percy here.

MuazOthman · 2022-05-25T14:56:42Z

@ZachJW34 the issue you found is now fixed thanks to @tbiethman. Can you please verify the fix, @ZachJW34?

MuazOthman · 2022-05-25T14:57:58Z

@marktnoonan file removed.

marktnoonan · 2022-05-25T15:03:28Z

packages/app/src/specs/SpecsList.vue

@@ -43,7 +43,9 @@
          t('specPage.componentSpecsHeader') : t('specPage.e2eSpecsHeader') }}
      </div>
      <div class="flex items-center justify-between">
-        <div>{{ t('specPage.lastUpdatedHeader') }}</div>
+        <div :is-git-available="isGitAvailable">


Looks like we can remove this attribute from the div and the isGitAvailable computed value, then this file won't be touched by the PR.

I removed that, but the localized string path was updated so this will touch the file.

rachelruderman · 2022-05-25T15:16:38Z

packages/app/src/specs/SpecListGitInfo.vue

    data-cy="git-info-row"
  >
    <Tooltip
+      v-if="classes.icon"
+      :key="props.gql?.statusType ?? undefined"


What is the key property used for here?

It seems that the Tooltip component from floating-vue binds events to its contents when it is mounted, which we're losing when the contents change, causing the tooltip not to show up.
Adding the key attribute and causing it to change value when the target changes forces Vue to re-render the DOM, which allows for the events to be bound correctly. This may not be an ideal solution, but given the use case here it doesn't create much overhead as these changes are triggered by file status changes, which are not expected to be frequent within 60 seconds, which is the frequency used to update the file status icon.
This article explains some more details about key in Vue. In summary:

The key attribute tells Vue how your data relates to the HTML elements it's rendering to the screen. When your data changes, Vue uses these keys to know which HTML elements to remove or update, and if it needs to create any new ones.

Yeah a key seems like the right move here 👍

@ZachJW34 this was the fix for the issue you found

rachelruderman · 2022-05-25T15:18:56Z

packages/app/src/specs/SpecListGitInfo.vue

+      icon: null,
+      iconClasses: 'icon-light-gray-500',


When the icon is null, do we still need to set iconClasses?

Good catch, removed iconClasses for this case.

rachelruderman · 2022-05-25T15:20:33Z

packages/app/src/specs/SpecListGitInfo.vue

-  return ''
+  return null


How come we changed the return type from string to string or null?

I guess this was me missing on how JS evaluates falsy strings 😆 . I'm reverting it.

rachelruderman · 2022-05-25T15:22:21Z

packages/data-context/src/sources/GitDataSource.ts

    } catch {
-      this.#git = simpleGit()
+      // suppress exception if git cannot be found


I hit another spot in the code where an exception was suppressed and it took me foreverrrrrsssss to track it down. Is there any way we can surface an error, i.e. the error still shows in the console, but it does not impact the UI?

Maybe a friendly neighborhood debug would work

debug added

marktnoonan · 2022-05-25T15:22:28Z

packages/data-context/src/sources/GitDataSource.ts

@@ -1,5 +1,5 @@
 import execa from 'execa'


Could you add something in the PR description that explains these changes in GitDataSource? It looks like this supports the "no git info" condition, but what's the expected change in the UI before and after? I found the info in the comments of the ticket that is linked in the PR, but that won't help future folks or outside contributors understand the changes, especially since we squash and merge PRs so the commit message that goes with these changes won't be directly visible. If we can move that comment from the ticket into the PR description I think it would help.

Can we have a test that simulates the no git info state and expected contents of the column based on file system info?

I added a new test to cover this case

Just saw this test fail for Windows, investigating.

rachelruderman · 2022-05-25T15:23:09Z

packages/data-context/src/sources/GitDataSource.ts

+      const toAwait = [
+        this.#loadAndWatchCurrentBranch(),
+      ]


rachelruderman · 2022-05-25T15:25:37Z

packages/data-context/src/sources/GitDataSource.ts


        let toSet: GitInfo | null = null

+        const stat = fs.statSync(file)
+        const ctime = dayjs(stat.ctime)
+        const birthtime = dayjs(stat.birthtime)


marktnoonan · 2022-05-25T15:26:21Z

packages/frontend-shared/src/locales/en-US.json

-    "lastUpdatedHeader": "Last updated",
+    "lastUpdated": {
+      "header": "Last updated",
+      "tooltip": {


We don't need this tooltip text in the i18n just yet, might as well remove it until the PR with the header updates comes along? Not a blocking request though.

Removed 👍

…-576-latest-update-design-updates

marktnoonan

I see a couple of tasks left:

Update this test mentioned here: https://github.com/cypress-io/cypress/pull/21536/files#r880715651. At the moment, if I completely remove the tooltip from SpecListGitInfo.vue, the specs_list_actual_git_repo.cy.ts test still passes, which seems wrong to me, it should be yelling about the tooltips being missing. Ideally the main tooltip with the git commit info would also be covered here, but I know that's not part of this PR, so it's your call whether to add it or not.
Update PR description to explain what's expected in the "no git info" state to provide context for the GitDataSource changes.

And just a side note, is the right box ticked here? Not familiar with our ZenHub tagging process but wondering if you meant to check the top box instead.

ZachJW34 · 2022-05-26T15:25:14Z

@MuazOthman verified that the tooltip is acting how it should! I'm curious, what was the fix?

rachelruderman · 2022-05-26T18:59:07Z

packages/app/cypress/tasks/git.ts

+  await git.reset(['--hard'])
+
+  return null


How come you're returning null? Is TS requiring you to have a return value?

I guess I stole the pattern from the function above it. Upon more digging, it seems it is required to return something per the comment on the code shown below

cypress/cli/types/cypress.d.ts

Lines 5472 to 5477 in d6df106

/**

* Individual task callback. Receives a single argument and _should_ return

* anything but `undefined` or a promise that resolves anything but `undefined`

* TODO: find a way to express "anything but undefined" in TypeScript

*/

type Task = (value: any) => any

…-576-latest-update-design-updates

marktnoonan

All looks great 👍

cypress-bot · 2022-06-01T15:48:56Z

Released in 10.0.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v10.0.0, please open a new issue.

fix: Spec List: Added tooltips to git icons created/modified

19d30fb

jennifer-shehane assigned MuazOthman May 18, 2022

emilyrohrbough reviewed May 18, 2022

View reviewed changes

packages/app/src/specs/SpecListGitInfo.vue Show resolved Hide resolved

emilyrohrbough reviewed May 18, 2022

View reviewed changes

WIP: Spec list - Last updated - no git

e044778

MuazOthman requested a review from tgriesser as a code owner May 20, 2022 17:21

tgriesser requested changes May 20, 2022

View reviewed changes

MuazOthman added 4 commits May 20, 2022 16:15

Fixed GitDataSource tests

067df92

Corrected no_git_info to noGitInfo

b8884b5

Merge remote-tracking branch 'origin/10.0-release' into muaz/CLOUD-57…

13ec47e

…6-latest-update-design-updates

Added tooltip for Last updated

e4e2b77

MuazOthman requested review from emilyrohrbough, tgriesser and rachelruderman May 23, 2022 14:46

tgriesser requested a review from marktnoonan May 23, 2022 21:38

MuazOthman added 2 commits May 24, 2022 09:39

Merge remote-tracking branch 'remotes/origin/10.0-release' into muaz/…

6622f6a

…CLOUD-576-latest-update-design-updates

Fixed a broken test

cc688f7

marktnoonan requested changes May 24, 2022

View reviewed changes

marktnoonan reviewed May 25, 2022

View reviewed changes

MuazOthman added 2 commits May 25, 2022 08:38

Removed tooltip for Last updated

ab89fbd

Cleaned up some code

c4ff1db

Code cleanup

12b1a62

marktnoonan reviewed May 25, 2022

View reviewed changes

Code cleanup

19ffae6

rachelruderman reviewed May 25, 2022

View reviewed changes

marktnoonan reviewed May 25, 2022

View reviewed changes

rachelruderman reviewed May 25, 2022

View reviewed changes

marktnoonan reviewed May 25, 2022

View reviewed changes

MuazOthman added 2 commits May 25, 2022 16:01

Addressed PR feedback

fb300d1

Merge remote-tracking branch 'remotes/origin/develop' into muaz/CLOUD…

ac45195

…-576-latest-update-design-updates

MuazOthman requested review from marktnoonan and rachelruderman May 25, 2022 21:32

marktnoonan reviewed May 26, 2022

View reviewed changes

Added tests for git tooltips

f9b5e43

MuazOthman requested a review from marktnoonan May 26, 2022 18:01

rachelruderman reviewed May 26, 2022

View reviewed changes

Merge remote-tracking branch 'remotes/origin/develop' into muaz/CLOUD…

d6df106

…-576-latest-update-design-updates

marktnoonan approved these changes May 26, 2022

View reviewed changes

rachelruderman self-requested a review May 27, 2022 16:05

rachelruderman approved these changes May 27, 2022

View reviewed changes

MuazOthman merged commit 0a6391f into develop May 27, 2022

MuazOthman deleted the muaz/CLOUD-576-latest-update-design-updates branch May 27, 2022 16:15

cypress-bot bot locked as resolved and limited conversation to collaborators Jun 1, 2022

	/**
	* Individual task callback. Receives a single argument and _should_ return
	* anything but `undefined` or a promise that resolves anything but `undefined`
	* TODO: find a way to express "anything but undefined" in TypeScript
	*/
	type Task = (value: any) => any

fix: Spec List: Added tooltips to git icons created/modified #21536

fix: Spec List: Added tooltips to git icons created/modified #21536

Conversation

MuazOthman commented May 17, 2022 • edited Loading

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented May 17, 2022

cypress bot commented May 17, 2022 • edited Loading

Test summary

Run details

marktnoonan commented May 17, 2022

ZachJW34 commented May 17, 2022

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

MuazOthman commented May 18, 2022

ZachJW34 commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MuazOthman commented May 23, 2022

marktnoonan left a 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

marktnoonan May 25, 2022 • edited Loading

Choose a reason for hiding this comment

MuazOthman commented May 25, 2022

MuazOthman commented May 25, 2022

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

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

marktnoonan left a comment

Choose a reason for hiding this comment

ZachJW34 commented May 26, 2022

Choose a reason for hiding this comment

MuazOthman May 26, 2022 • edited Loading

Choose a reason for hiding this comment

marktnoonan left a comment

Choose a reason for hiding this comment

cypress-bot bot commented Jun 1, 2022

MuazOthman commented May 17, 2022 •

edited

Loading

cypress bot commented May 17, 2022 •

edited

Loading

marktnoonan May 25, 2022 •

edited

Loading

MuazOthman May 26, 2022 •

edited

Loading