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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Feb 10, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a493725
(i18n) add locale strings for the "view as" banner
manuhabitela Feb 3, 2025
8393b1a
(i18n) use existing translation strings for acl roles
manuhabitela Feb 3, 2025
8ecfe3c
(acl) adding a tooltip helping user understand the "view as" screen
manuhabitela Feb 3, 2025
0d7b3fb
(i18n) adding french translations for ViewAsBanner
manuhabitela Feb 3, 2025
cbd2813
(acl) in "view as" banner, fix eye icon color in dark theme
manuhabitela Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion app/client/aclui/ACLUsers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {waitGrainObs} from 'app/common/gutil';
import noop from 'lodash/noop';

const t = makeT("ViewAsDropdown");
const userT = makeT('UserManagerModel');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@manuhabitela manuhabitela Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

image


function isSpecialEmail(email: string) {
return email === ANONYMOUS_USER_EMAIL || email === EVERYONE_EMAIL;
Expand Down Expand Up @@ -127,7 +128,7 @@ export class ACLUsersPopup extends Disposable {
),
cssMemberText(
cssMemberPrimary(user.name || dom('span', user.email),
cssRole('(', getUserRoleText(user), ')', testId('acl-user-access')),
cssRole('(', userT(getUserRoleText(user)), ')', testId('acl-user-access')),
),
user.name ? cssMemberSecondary(user.email) : null
),
Expand Down
3 changes: 2 additions & 1 deletion app/client/components/Banner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ const cssBanner = styled('div', `
color: white;

&-info {
color: black;
color: ${colors.dark};
--icon-color: ${colors.dark};
background: #FFFACD;
}

Expand Down
25 changes: 18 additions & 7 deletions app/client/components/ViewAsBanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ import { cssSelectBtn } from 'app/client/ui2018/select';
import { ACLUsersPopup } from 'app/client/aclui/ACLUsers';
import { UserOverride } from 'app/common/DocListAPI';
import { makeT } from 'app/client/lib/localization';
import { cssInfoTooltipButton, withInfoTooltip } from 'app/client/ui/tooltips';

const t = makeT('components.ViewAsBanner');
const t = makeT('ViewAsBanner');
const userT = makeT('UserManagerModel');

export class ViewAsBanner extends Disposable {

Expand Down Expand Up @@ -45,14 +47,14 @@ export class ViewAsBanner extends Disposable {
return cssContent(
cssMessageText(
cssMessageIcon('EyeShow'),
'You are viewing this document as',
t('You are viewing this document as'),
),
cssSelectBtn(
{tabIndex: '0'},
cssBtnText(
user ? cssMember(
user.name || user.email,
cssRole('(', getUserRoleText({...user, access}), ')', dom.show(Boolean(access))),
cssRole('(', userT(getUserRoleText({...user, access})), ')', dom.show(Boolean(access))),
) : t('UnknownUser'),
),
dom(
Expand All @@ -62,10 +64,19 @@ export class ViewAsBanner extends Disposable {
elem => this._usersPopup.attachPopup(elem, {}),
testId('select-open'),
),
cssPrimaryButtonLink(
'View as Yourself', cssIcon('Convert'),
urlState().setHref(userOverrideParams(null)),
testId('revert'),
withInfoTooltip(
cssPrimaryButtonLink(
t('View as Yourself'), cssIcon('Convert'),
urlState().setHref(userOverrideParams(null)),
testId('revert'),
),
'viewAsBanner',
{
iconDomArgs: [
cssInfoTooltipButton.cls('-in-banner'),
testId('view-as-help-tooltip'),
],
},
),
testId('view-as-banner'),
);
Expand Down
7 changes: 6 additions & 1 deletion app/client/ui/GristTooltips.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ export type Tooltip =
| 'communityWidgets'
| 'twoWayReferences'
| 'twoWayReferencesDisabled'
| 'reasignTwoWayReference';
| 'reasignTwoWayReference'
| 'viewAsBanner';

export type TooltipContentFunc = (...domArgs: DomElementArg[]) => DomContents;

Expand Down Expand Up @@ -187,6 +188,10 @@ see or edit which parts of your document.')
),
...args,
),
viewAsBanner: (...args: DomElementArg[]) => cssTooltipContent(
dom('div', t('The preview below this header shows how the selected user will see this document')),
...args,
),
};

export interface BehavioralPromptContent {
Expand Down
16 changes: 13 additions & 3 deletions app/client/ui/tooltips.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import {logTelemetryEvent} from 'app/client/lib/telemetry';
import {GristTooltips, Tooltip} from 'app/client/ui/GristTooltips';
import {prepareForTransition} from 'app/client/ui/transitions';
import {testId, theme, vars} from 'app/client/ui2018/cssVars';
import {colors, testId, theme, vars} from 'app/client/ui2018/cssVars';
import {icon} from 'app/client/ui2018/icons';
import {makeLinks} from 'app/client/ui2018/links';
import {menuCssClass} from 'app/client/ui2018/menus';
Expand Down Expand Up @@ -401,7 +401,7 @@ function buildHoverableInfoTooltip(
tooltip: BindableValue<Tooltip>,
...domArgs: DomElementArg[]
) {
return cssInfoTooltipIcon('?',
return cssInfoTooltipButton('?',
hoverTooltip(() => cssInfoTooltipTransientPopup(
dom.domComputed(tooltip, (tip) => GristTooltips[tip]()),
cssTooltipCorner(testId('tooltip-origin')),
Expand Down Expand Up @@ -587,13 +587,23 @@ const cssInfoTooltipIcon = styled('div', `
}
`);

const cssInfoTooltipButton = styled(cssInfoTooltipIcon, `
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};
}
`);

const cssInfoTooltipPopup = styled('div', `
Expand Down
7 changes: 5 additions & 2 deletions static/locales/en.client.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -850,7 +850,9 @@
"Update formula (Shift+Enter)": "Update formula (Shift+Enter)"
},
"ViewAsBanner": {
"UnknownUser": "Unknown User"
"UnknownUser": "Unknown User",
"View as Yourself": "View as Yourself",
"You are viewing this document as": "You are viewing this document as"
},
"ViewConfigTab": {
"Advanced settings": "Advanced settings",
Expand Down Expand Up @@ -1237,7 +1239,8 @@
"To allow multiple assignments, change the type of the Reference column to Reference List.": "To allow multiple assignments, change the type of the Reference column to Reference List.",
"This limitation occurs when one column in a two-way reference has the Reference type.": "This limitation occurs when one column in a two-way reference has the Reference type.",
"To allow multiple assignments, change the referenced column's type to Reference List.": "To allow multiple assignments, change the referenced column's type to Reference List.",
"Two-way references are not currently supported for Formula or Trigger Formula columns": "Two-way references are not currently supported for Formula or Trigger Formula columns"
"Two-way references are not currently supported for Formula or Trigger Formula columns": "Two-way references are not currently supported for Formula or Trigger Formula columns",
"The preview below this header shows how the selected user will see this document": "The preview below this header shows how the selected user will see this document"
},
"DescriptionConfig": {
"DESCRIPTION": "DESCRIPTION"
Expand Down
7 changes: 5 additions & 2 deletions static/locales/fr.client.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,9 @@
"Users from table": "Utilisateurs de la table"
},
"ViewAsBanner": {
"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"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

},
"CellStyle": {
"Open row styles": "Ouvrir les styles de ligne",
Expand Down Expand Up @@ -1220,7 +1222,8 @@
"To allow multiple assignments, change the referenced column's type to Reference List.": "Pour permettre plusieurs assignations, modifiez le type de la colonne référencée en Référence Multiple.",
"To allow multiple assignments, change the type of the Reference column to Reference List.": "Pour permettre des assignations multiples, modifiez le type de la colonne Référence en Référence Multiple.",
"This limitation occurs when one end of a two-way reference is configured as a single Reference.": "Cette limitation se produit lorsque l'une des extrémités d'une référence bidirectionnelle est configurée comme une référence unique.",
"Two-way references are not currently supported for Formula or Trigger Formula columns": "Les références bidirectionnelles ne peuvent pas contenir des formules ou des formules d'initialisation"
"Two-way references are not currently supported for Formula or Trigger Formula columns": "Les références bidirectionnelles ne peuvent pas contenir des formules ou des formules d'initialisation",
"The preview below this header shows how the selected user will see this document": "L'aperçu sous cette entête montre ce que l'utilisateur sélectionné peut voir"
},
"ColumnTitle": {
"Add description": "Ajouter une description",
Expand Down
1 change: 1 addition & 0 deletions test/nbrowser/AccessRules2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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').isDisplayed(), true);

// check the aclAsUser parameter on the url persists after navigating to another page
await gu.getPageItem('FinancialsTable').click();
Expand Down
Loading