Skip to content

Rename "New" files back - add Tooltip to ws branch #16397

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

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

selfcontained
Copy link
Contributor

Description

  • Cleans up some filenames to remove the New from them after refactoring the Workspaces list components.
  • Added a tooltip to the branch name as a small improvement for when it's cut off and you can't tell what the branch is.

How to test

  • Verify Workspaces list view renders correctly.
  • Hover over the branch name in the workspaces list and ensure there's a tooltip.

Release Notes

NONE

Documentation

Build Options:

  • /werft with-github-actions
    Experimental feature to run the build with GitHub Actions (and not in Werft).
  • leeway-no-cache
    leeway-target=components:all
  • /werft no-test
    Run Leeway with --dont-test
Publish Options
  • /werft publish-to-npm
  • /werft publish-to-jb-marketplace
Installer Options
  • with-ee-license
  • with-slow-database
  • with-dedicated-emulation
  • with-ws-manager-mk2
  • workspace-feature-flags
    Add desired feature flags to the end of the line above, space separated

Preview Environment Options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-large-vm
  • /werft with-gce-vm
    If enabled this will create the environment on GCE infra
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide, jetbrains, vscode, ssh

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-bmh-workspace-entry-tweaks.1 because the annotations in the pull request description changed
(with .werft/ from main)

@selfcontained
Copy link
Contributor Author

selfcontained commented Feb 14, 2023

/werft run with-preview with-dedicated-emulation with-clean-slate-deployment recreate-vm

👍 started the job as gitpod-build-bmh-workspace-entry-tweaks.2
(with .werft/ from main)

@selfcontained selfcontained changed the title Bmh/workspace-entry-tweaks Rename "New" files back - add Tooltip to ws branch Feb 14, 2023
@selfcontained
Copy link
Contributor Author

selfcontained commented Feb 14, 2023

/werft run with-preview with-clean-slate-deployment recreate-vm

👍 started the job as gitpod-build-bmh-workspace-entry-tweaks.3
(with .werft/ from main)

@selfcontained selfcontained marked this pull request as ready for review February 14, 2023 22:58
@selfcontained selfcontained requested a review from a team February 14, 2023 22:58
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Feb 14, 2023
Copy link
Member

@svenefftinge svenefftinge left a comment

Choose a reason for hiding this comment

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

LGTM
/hold would be good to squash the commits

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

UX LGTM!

Approving to unblock merging but holding for squashing the commits[1].

/hold

@@ -74,7 +74,9 @@ export const WorkspaceEntry: FunctionComponent<Props> = ({ info }) => {
</a>
</ItemField>
<ItemField className="w-2/12 flex flex-col my-auto">
<div className="text-gray-500 dark:text-gray-400 overflow-ellipsis truncate">{currentBranch}</div>
<div className="text-gray-500 dark:text-gray-400 overflow-ellipsis truncate">
<Tooltip content={currentBranch}>{currentBranch}</Tooltip>
Copy link
Contributor

Choose a reason for hiding this comment

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

Some raw thoughts on this change:

  1. It's better to avoid as many tooltips as possible, mostly for accessibility.
  2. Tooltip seems redundant when the branch is not truncated.
  3. Tooltip seems redundant when the default branch is by convention short like master or main.
  4. This could use the title attribute as a fallback instead.
  5. The tooltip will become less needed with changes in Improve workspaces page #12033, see also screenshot below.
  6. Following up from the changes in Improve workspaces page #12033 the branch could be truncated in some edge cases but still be accompanied by a copy-to-clipboard button next to the branch name.
Early design for improving the workspaces list
Workspaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah the new design will help so we wouldn't need something like this. My motivation is that I end up with the following situation:

image

and the only way I can tell what's what is by closing any sidebars, and hoping it doesn't clip (and still does sometimes). This seemed like a reasonable / easy way to offer a recourse for a user to see their branch.

I'm not sure I follow what accessibility concerns there would be with this. Using our custom tooltip component is essentially the same behavior as using title, but a slightly better ux.

I do agree it is redundant, and shouldn't be needed (and won't in new designs whenever we can do them). At the moment, I was hoping to improve it slightly until we do have time to work on the new designs. It's easy enough to remove, so I will if you still think we should, but also feels like a quality of life improvement that I know I'd benefit from, so I assume some others will to.

Copy link
Contributor

@gtsiolis gtsiolis Feb 16, 2023

Choose a reason for hiding this comment

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

@selfcontained I'm coming to this a bit late to share some thoughts on this topic—I also understand where you are coming from and I'm looking forward to improving the bits in this page.

I'm not sure I follow what accessibility concerns there would be with this. Using our custom tooltip component is essentially the same behavior as using title, but a slightly better ux.

To clarify, I believe there are cases where tooltips can be a helpful and valuable component for surfacing information like describing a button action, provide a hint, etc. However, it's easy to skip making it accessible, as tooltips that only appear on mouse over are inaccessible for users that rely on keyboards to navigate. This is a perfect example as the branch element is not accessible by keyboard and the tooltip never shows up.

I do agree it is redundant, and shouldn't be needed (and won't in new designs whenever we can do them).

💯

... I was hoping to improve it slightly until we do have time to work on the new designs. It's easy enough to remove, so I will if you still think we should, but also feels like a quality of life improvement that I know I'd benefit from, so I assume some others will to.

Sounds like a good MVC we can easily update or revert when we start working on the new page designs. Also, since the changes in #15716 this seems like ok to leave it there for now. 🤝


Also, regarding the comment below in #16397 (comment):

I noticed something with tooltip change I mad here makes it hard to hover over the tip and select/copy the value now.

Since a tooltip disappears when a user hovers away, we should not include information that is pertinent for the user to complete a task. We should instead rely on other patterns or components for making vital information always visible and accessible, like help text, labels, etc.

@selfcontained selfcontained force-pushed the bmh/workspace-entry-tweaks branch from 49edee7 to 9739006 Compare February 15, 2023 15:35
@selfcontained
Copy link
Contributor Author

I noticed something with tooltip change I mad here makes it hard to hover over the tip and select/copy the value now. I'll figure that out before merging this.

@roboquat roboquat added size/M and removed size/S labels Feb 15, 2023
@selfcontained
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit d4f24d6 into main Feb 15, 2023
@roboquat roboquat deleted the bmh/workspace-entry-tweaks branch February 15, 2023 21:49
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Feb 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants