"view as" banner: add a tooltip so that users understand better what the screen does #1418

manuhabitela · 2025-02-03T18:37:57Z

Context

Fixes #1328.

Proposed solution

I'm not that familiar with grain-js and how translation strings are handled in the project still so I hope I coded this the good way!
I allowed myself to add translation strings for this component as they were missing.

Has this been tested?

Added a very small test, just making sure the new tooltip icon is there. Should I be more specific?

👍 yes, I added tests to the test suite
💭 no, because this PR is a draft and still needs work
🙅 no, because this is not relevant here
🙋 no, because I need help

Screenshots / Screencasts

manuhabitela · 2025-02-03T18:39:17Z

app/client/aclui/ACLUsers.ts

@@ -18,6 +18,7 @@ import {waitGrainObs} from 'app/common/gutil';
 import noop from 'lodash/noop';

 const t = makeT("ViewAsDropdown");
+const userT = makeT('UserManagerModel');


I allowed myself to use translation strings that are "out" of the components (here in ACLUser and below in ViewAsBanner), as they fit the use-case. Is it okay?

I added a preview tag to the PR to have a way to easily check the result of this userT() when app is in french.

It looks like this below.

The "Owner", "Editor 1", "Editor 2" fake user names are hardcoded strings that are not in the translation system, I figured it was good enough as is with the now translated actual roles.

manuhabitela · 2025-02-03T18:39:51Z

static/locales/fr.client.json

-        "UnknownUser": "Utilisateur inconnu"
+        "UnknownUser": "Utilisateur inconnu",
+        "View as Yourself": "Voir en tant que vous-même",
+        "You are viewing this document as": "Vous voyez ce document en tant que"


I directly updated the fr locale file instead of going through weblate. Is that okay?

IMO it is good as it does the same than the weblate result.
If I'm wrong please correct me dear gristlabs fellows.

github-actions · 2025-02-03T19:23:17Z

Deployed commit a03d366c29923f22d7b1df90df3466f862a2f8b4 as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-05T19:21:14.371Z)

hexaltation

Hello
and thank you for the job.

A little ask for a change :

When over the tooltip, as it do not really change it's not very clear that it's a clickable element. (seems to imperceptibly change from really dark gray to black)

Elsewhere LGTM

manuhabitela · 2025-02-04T10:22:16Z

When over the tooltip, as it do not really change it's not very clear that it's a clickable element. (seems to imperceptibly change from really dark gray to black)

Yeah I originally put a lighter gray color but ended up just following the mockup. What do you think Lucie? I can change to do something like this:

hover.mp4

manuhabitela · 2025-02-04T10:24:51Z

Stumbled upon a tiny color bug in dark theme on the eye icon on the left, allowed myself to add the small fix in this PR 👀

hexaltation · 2025-02-04T10:27:09Z

Yeah I originally put a lighter gray color but ended up just following the mockup. What do you think @lusebille ? I can change to do something like this:
hover.mp4

I had lucie's pseudo so she will have a notification

github-actions · 2025-02-04T10:31:16Z

Deployed commit e56acd54894c0101c23e6b9331c84c97175740a5 as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-06T10:29:17.737Z)

manuhabitela · 2025-02-04T13:50:46Z

After talking with Lucie:

I kept the blackish icon color, to have this interactive icon more pronounced than the non-interactive, gray eye on the left.
And I set the hover color to be green to better tell it's interactive.

Right now the green on yellow is not that great contrast-wise but this will change soon with the wcag-compliance changes planned after the themes rework.

github-actions · 2025-02-04T13:55:50Z

Deployed commit 32300a78458162d7da210046e00b721a338345cf as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-06T13:54:14.069Z)

hexaltation

LGTM
Thanks :)

dsagal

Thanks @manuhabitela ! Looks good overall, but I have a couple comments about colors.

app/client/components/ViewAsBanner.ts

dsagal · 2025-02-06T04:54:12Z

app/client/ui/tooltips.ts

@@ -596,6 +612,17 @@ const cssInfoTooltipButton = styled(cssInfoTooltipIcon, `
  }
 `);

+/* styling variant for tooltips displayed in info banners (where background is light yellow) */
+export const cssInfoTooltipButtonInBannerInfo = styled(cssInfoTooltipIcon, `


I wonder if we actually want the style of the tooltip icon in the banner to be different from that elsewhere. I see that contrast is poor with the current style, but it's as poor in the banner as in the creator panel. It seems best to me to either stick to the same style as currently used elsewhere (and worry about contrast later), or to update that style to have more contrast everywhere now (perhaps the dark + green-on-hover that you have for the banner).

Wdyt?

I added this because the banner, contrary to most other places like the creator panel, doesn't change colors when in dark mode. Without this specific style, it then looks like this:

tooltip-colors.mp4

And if I applied this new style everywhere, tooltips would now be barely visible in most places in dark theme.

Instead of creating this tooltip variant, I could just update the Banner css, like for the --icon-color stuff above. But I felt it started to be weird having a few tooltip-related styles in a totally different part of the code, instead of being explicit and creating an actual tooltip style variant.

Or maybe I could rename this style variant to be more generic and not namely tied to the banner, like 'always-dark'?

Now it looks like this:

newtooltip.mp4

Maybe I'm missing something and this could be done in another way?

I understand the issue now, thanks for explaining. Yes, there is a simpler way, to avoid passing a new argument through so much of the code. Since there is already an argument to customize style of the tooltip button using iconDomArgs (which I see you using in this revision), you can add style variants like so:

In ViewAsBanner.ts:

import { cssInfoTooltipButton, withInfoTooltip } from 'app/client/ui/tooltips'; withInfoTooltip( cssPrimaryButtonLink(...), 'viewAsBanner', { iconDomArgs: [ cssInfoTooltipButton.cls('-in-banner'), // <-- this is what adds a css class variant to the button testId('view-as-help-tooltip'), ], ...)

In tooltips.ts:

export const cssInfoTooltipButton = styled(cssInfoTooltipIcon, ` cursor: pointer; &:hover { border: 1px solid ${theme.controlSecondaryHoverFg}; color: ${theme.controlSecondaryHoverFg}; } &-in-banner { color: ${colors.dark}; border-color: ${colors.dark}; } &-in-banner:hover { border-color: ${colors.lightGreen}; color: ${colors.lightGreen}; } `);

The &-in-banner blocks define this variant. It's simply another CSS class applied to the tooltip-button element.

Ok, why not :)

Feels like the solution with more code allowed for more isolated code, like you don't have to know about the internals, the specific classes to apply; you just know you can apply a different look. But for sure this solution is more straightforward for this pretty specific case.

manuhabitela · 2025-02-07T10:58:02Z

I added a very small test, just making sure the new help tooltip is there in the DOM.

Figured I wouldn't test the actual tooltip behavior here. Should I make my test more specific? Ie making sure it's clickable, have correct help text…?

Some tests don't pass (Importer), pretty sure they are completely unrelated to this PR code?

github-actions · 2025-02-07T11:02:06Z

Deployed commit 09340b68df0d15d2d0e8506fcc01832c188c39ee as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-09T11:00:36.645Z)

github-actions · 2025-02-07T11:16:07Z

Deployed commit 074845301fd6d66031255cfe9599916f88f2a8b3 as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-09T11:14:42.494Z)

dsagal · 2025-02-09T21:59:18Z

app/client/components/Banner.ts

@@ -122,6 +122,7 @@ const cssBanner = styled('div', `

  &-info {
    color: black;


I suggest changing this to ${colors.dark} along the way, for consistency.

dsagal · 2025-02-09T22:45:07Z

app/client/ui/tooltips.ts

@@ -596,6 +612,17 @@ const cssInfoTooltipButton = styled(cssInfoTooltipIcon, `
  }
 `);

+/* styling variant for tooltips displayed in info banners (where background is light yellow) */
+export const cssInfoTooltipButtonInBannerInfo = styled(cssInfoTooltipIcon, `


I understand the issue now, thanks for explaining. Yes, there is a simpler way, to avoid passing a new argument through so much of the code. Since there is already an argument to customize style of the tooltip button using iconDomArgs (which I see you using in this revision), you can add style variants like so:

In ViewAsBanner.ts:

import { cssInfoTooltipButton, withInfoTooltip } from 'app/client/ui/tooltips'; withInfoTooltip( cssPrimaryButtonLink(...), 'viewAsBanner', { iconDomArgs: [ cssInfoTooltipButton.cls('-in-banner'), // <-- this is what adds a css class variant to the button testId('view-as-help-tooltip'), ], ...)

In tooltips.ts:

export const cssInfoTooltipButton = styled(cssInfoTooltipIcon, ` cursor: pointer; &:hover { border: 1px solid ${theme.controlSecondaryHoverFg}; color: ${theme.controlSecondaryHoverFg}; } &-in-banner { color: ${colors.dark}; border-color: ${colors.dark}; } &-in-banner:hover { border-color: ${colors.lightGreen}; color: ${colors.lightGreen}; } `);

The &-in-banner blocks define this variant. It's simply another CSS class applied to the tooltip-button element.

dsagal · 2025-02-09T22:50:02Z

test/nbrowser/AccessRules2.ts

@@ -109,6 +109,7 @@ describe("AccessRules2", function() {
    assert.equal(await driver.findWait('.test-view-as-banner', 2000).isPresent(), true);
    assert.match(await driver.find('.test-view-as-banner .test-select-open').getText(),
                 new RegExp(gu.translateUser('user3').name, 'i'));
+    assert.equal(await driver.find('.test-info-tooltip.test-view-as-help-tooltip').isPresent(), true);


You can change isPresent to isDisplayed to make this check a tiny bit stricter: isPresent only checks if an element is present in the DOM, while isDisplayed checks that it's visible (e.g. doesn't have display: none style).

manuhabitela · 2025-02-10T11:37:50Z

Thanks @dsagal, updated with your feedback (will just need a rebase to merge the fixup commit correctly).

github-actions · 2025-02-10T11:42:48Z

Deployed commit ff18f89ca7b912a72863906e7bcd9ad12f15e842 as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-12T11:41:19.711Z)

dsagal

Looks good, thank you @manuhabitela !

in the "view as" banner, the role strings (owner, editor, viewer) were not translated and we can re-use the ones defined in the UserManagerModel (i guess?).

eye icon was light grey on yellow in dark theme, making it basically invisible, forcing dark color all the time

manuhabitela · 2025-02-10T16:56:37Z

Thanks, I rebased and cleaned the commits 👌

github-actions · 2025-02-10T17:02:02Z

Deployed commit cbd281348e1f1759ce74da5cc6ffb8a452f46e4d as https://grist-manuhabitela-grist-core-issue-1328-view-as-info.fly.dev (until 2025-03-12T17:00:31.082Z)

manuhabitela changed the title ~~Issue 1328 view as info~~ "view as" banner: add a tooltip so that users understand better what the screen does Feb 3, 2025

manuhabitela commented Feb 3, 2025

View reviewed changes

manuhabitela added the gouv.fr label Feb 3, 2025

manuhabitela requested a review from hexaltation February 3, 2025 18:40

hexaltation added the preview Launch preview deployment of this PR label Feb 3, 2025

hexaltation suggested changes Feb 3, 2025

View reviewed changes

manuhabitela force-pushed the issue-1328-view-as-info branch from e56acd5 to 32300a7 Compare February 4, 2025 13:49

hexaltation approved these changes Feb 4, 2025

View reviewed changes

manuhabitela self-assigned this Feb 5, 2025

dsagal reviewed Feb 6, 2025

View reviewed changes

manuhabitela force-pushed the issue-1328-view-as-info branch from 32300a7 to 09340b6 Compare February 7, 2025 10:55

manuhabitela force-pushed the issue-1328-view-as-info branch from 09340b6 to 0748453 Compare February 7, 2025 11:09

dsagal reviewed Feb 9, 2025

View reviewed changes

manuhabitela force-pushed the issue-1328-view-as-info branch from 0748453 to ff18f89 Compare February 10, 2025 11:36

dsagal approved these changes Feb 10, 2025

View reviewed changes

manuhabitela added 3 commits February 10, 2025 17:54

(i18n) add locale strings for the "view as" banner

a493725

(i18n) use existing translation strings for acl roles

8393b1a

in the "view as" banner, the role strings (owner, editor, viewer) were not translated and we can re-use the ones defined in the UserManagerModel (i guess?).

(acl) adding a tooltip helping user understand the "view as" screen

8ecfe3c

manuhabitela added 2 commits February 10, 2025 17:55

(i18n) adding french translations for ViewAsBanner

0d7b3fb

(acl) in "view as" banner, fix eye icon color in dark theme

cbd2813

eye icon was light grey on yellow in dark theme, making it basically invisible, forcing dark color all the time

manuhabitela force-pushed the issue-1328-view-as-info branch from ff18f89 to cbd2813 Compare February 10, 2025 16:55

dsagal merged commit c4b4b53 into gristlabs:main Feb 10, 2025
11 of 12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

"view as" banner: add a tooltip so that users understand better what the screen does #1418

"view as" banner: add a tooltip so that users understand better what the screen does #1418

manuhabitela commented Feb 3, 2025 •

edited

Loading

manuhabitela Feb 3, 2025

hexaltation Feb 3, 2025

manuhabitela Feb 4, 2025 •

edited

Loading

manuhabitela Feb 3, 2025

hexaltation Feb 3, 2025

github-actions bot commented Feb 3, 2025

hexaltation left a comment

manuhabitela commented Feb 4, 2025

manuhabitela commented Feb 4, 2025

hexaltation commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

manuhabitela commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

hexaltation left a comment

dsagal left a comment

dsagal Feb 6, 2025

manuhabitela Feb 7, 2025

dsagal Feb 9, 2025

manuhabitela Feb 10, 2025

manuhabitela commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025

dsagal Feb 9, 2025

dsagal Feb 9, 2025

dsagal Feb 9, 2025

manuhabitela commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

dsagal left a comment

manuhabitela commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

		@@ -122,6 +122,7 @@ const cssBanner = styled('div', `

		&-info {
		color: black;

"view as" banner: add a tooltip so that users understand better what the screen does #1418

"view as" banner: add a tooltip so that users understand better what the screen does #1418

Conversation

manuhabitela commented Feb 3, 2025 • edited Loading

Context

Proposed solution

Has this been tested?

Screenshots / Screencasts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuhabitela Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025

hexaltation left a comment

Choose a reason for hiding this comment

manuhabitela commented Feb 4, 2025

manuhabitela commented Feb 4, 2025

hexaltation commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

manuhabitela commented Feb 4, 2025

github-actions bot commented Feb 4, 2025

hexaltation left a comment

Choose a reason for hiding this comment

dsagal 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

Choose a reason for hiding this comment

manuhabitela commented Feb 7, 2025 • edited Loading

github-actions bot commented Feb 7, 2025

github-actions bot commented Feb 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuhabitela commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

dsagal left a comment

Choose a reason for hiding this comment

manuhabitela commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

manuhabitela commented Feb 3, 2025 •

edited

Loading

manuhabitela Feb 4, 2025 •

edited

Loading

manuhabitela commented Feb 7, 2025 •

edited

Loading