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

Jump to bottom

feat: design changes in signature paged message section #28038

Merged
merged 20 commits into from
Oct 24, 2024
Merged

feat: design changes in signature paged message section #28038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8a24413
Design changes in signature paged message section
jpuri Oct 23, 2024
c68d6fb
merge
jpuri Oct 23, 2024
1181b8d
update
jpuri Oct 23, 2024
a699c9e
update
jpuri Oct 23, 2024
73f7b71
update
jpuri Oct 23, 2024
7a59656
update
jpuri Oct 23, 2024
94000e3
update
jpuri Oct 23, 2024
f2ed1c6
update
jpuri Oct 23, 2024
4af935c
update
jpuri Oct 23, 2024
b834e33
update
jpuri Oct 23, 2024
a9f7d46
update
jpuri Oct 24, 2024
440a8d3
update
jpuri Oct 24, 2024
c95f82d
update
jpuri Oct 24, 2024
c2aef7b
update
jpuri Oct 24, 2024
b6b4d59
update
jpuri Oct 24, 2024
8d31dad
update
jpuri Oct 24, 2024
f389d57
update
jpuri Oct 24, 2024
cd2c82f
update
jpuri Oct 24, 2024
ac11fc3
update
jpuri Oct 24, 2024
44ec97d
chore: optimizing svg for collapse icon svg
georgewrmarshall Oct 24, 2024
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
1 change: 1 addition & 0 deletions app/images/icons/collapse.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions test/e2e/tests/confirmations/signatures/permit.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ describe('Confirmation Signature - Permit @no-mmi', function (this: Suite) {
});

async function assertInfoValues(driver: Driver) {
await driver.clickElement('[data-testid="sectionCollapseButton"]');
const origin = driver.findElement({ text: DAPP_HOST_ADDRESS });
const contractPetName = driver.findElement({
css: '.name__value',
Expand Down
1 change: 1 addition & 0 deletions test/e2e/tests/confirmations/signatures/siwe.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ describe('Confirmation Signature - SIWE @no-mmi', function (this: Suite) {
});

async function assertInfoValues(driver: Driver) {
await driver.clickElement('[data-testid="sectionCollapseButton"]');
const origin = driver.findElement({ text: DAPP_HOST_ADDRESS });
const message = driver.findElement({
text: 'I accept the MetaMask Terms of Service: https://community.metamask.io/tos',
Expand Down
4 changes: 3 additions & 1 deletion test/e2e/tests/hardware-wallets/trezor-sign.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ describe('Trezor Hardware Signatures', function (this: Suite) {
await openDapp(driver);
await driver.clickElement('#signTypedDataV4');
await driver.switchToWindowWithTitle(WINDOW_TITLES.Dialog);
await driver.delay(1000);

await driver.clickElement('.confirm-scroll-to-bottom__button');
await driver.clickElementSafe('.confirm-scroll-to-bottom__button');
await driver.clickElement({ text: 'Confirm', tag: 'button' });
await driver.delay(1000);

await driver.waitUntilXWindowHandles(2);
await driver.switchToWindowWithTitle(WINDOW_TITLES.TestDApp);
Expand Down
4 changes: 2 additions & 2 deletions ui/components/app/confirm/info/row/__snapshots__/row.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ exports[`ConfirmInfoRow should match snapshot when copy is enabled 1`] = `
style="overflow-wrap: anywhere; min-height: 24px; position: relative;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-alternative"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 0px; top: 2px;"
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 0px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start"
Expand Down
18 changes: 14 additions & 4 deletions ui/components/app/confirm/info/row/copy-icon.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
import React, { useCallback } from 'react';
import React, { CSSProperties, useCallback } from 'react';

import { useCopyToClipboard } from '../../../../../hooks/useCopyToClipboard';
import { IconColor } from '../../../../../helpers/constants/design-system';
import { Icon, IconName, IconSize } from '../../../../component-library';

type CopyCallback = (text: string) => void;

export const CopyIcon: React.FC<{ copyText: string }> = ({ copyText }) => {
export const CopyIcon: React.FC<{
copyText: string;
color?: IconColor;
style?: CSSProperties;
}> = ({ copyText, color, style = {} }) => {
const [copied, handleCopy] = useCopyToClipboard();

const handleClick = useCallback(async () => {
Expand All @@ -15,10 +19,16 @@ export const CopyIcon: React.FC<{ copyText: string }> = ({ copyText }) => {

return (
<Icon
color={IconColor.iconAlternative}
color={color ?? IconColor.iconAlternative}
name={copied ? IconName.CopySuccess : IconName.Copy}
size={IconSize.Sm}
style={{ cursor: 'pointer', position: 'absolute', right: 0, top: 2 }}
style={{
cursor: 'pointer',
position: 'absolute',
right: 0,
top: 2,
...style,
}}
onClick={handleClick}
Copy link
Contributor

@georgewrmarshall georgewrmarshall Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

non-blocking: Is there any reason we aren't using the ButtonIcon component here? The current implementation has poor accessibility as it renders a span instead of a button, and it lacks an aria-label, making it inaccessible for screen reader users.

cc @matthewwalsh0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree button icon is good idea. Let me create separate PR for this as it would need lot of snapshot updates in confirmation pages.

/>
);
Expand Down
18 changes: 17 additions & 1 deletion ui/components/app/confirm/info/row/row.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React from 'react';
import { render } from '@testing-library/react';
import { fireEvent, render, screen } from '@testing-library/react';

import { Text } from '../../../../component-library';
import { ConfirmInfoRow } from './row';
Expand All @@ -22,4 +22,20 @@ describe('ConfirmInfoRow', () => {
);
expect(container).toMatchSnapshot();
});

it('should be expandable when collapsed is true', () => {
render(
<ConfirmInfoRow
label="some label"
copyEnabled
copyText="dummy text"
collapsed
>
<Text>Some text</Text>
</ConfirmInfoRow>,
);
expect(screen.queryByText('Some text')).not.toBeInTheDocument();
fireEvent.click(screen.getByTestId('sectionCollapseButton'));
expect(screen.queryByText('Some text')).toBeInTheDocument();
});
});
156 changes: 93 additions & 63 deletions ui/components/app/confirm/info/row/row.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { createContext } from 'react';
import React, { createContext, useState } from 'react';
import Tooltip from '../../../../ui/tooltip/tooltip';
import {
Box,
Expand Down Expand Up @@ -40,6 +40,7 @@ export type ConfirmInfoRowProps = {
copyEnabled?: boolean;
copyText?: string;
'data-testid'?: string;
collapsed?: boolean;
};

const BACKGROUND_COLORS = {
Expand Down Expand Up @@ -79,71 +80,100 @@ export const ConfirmInfoRow: React.FC<ConfirmInfoRowProps> = ({
labelChildren,
color,
copyEnabled = false,
copyText = undefined,
copyText,
'data-testid': dataTestId,
}) => (
<ConfirmInfoRowContext.Provider value={{ variant }}>
<Box
data-testid={dataTestId}
className="confirm-info-row"
display={Display.Flex}
flexDirection={FlexDirection.Row}
justifyContent={JustifyContent.spaceBetween}
flexWrap={FlexWrap.Wrap}
alignItems={AlignItems.center}
backgroundColor={BACKGROUND_COLORS[variant]}
borderRadius={BorderRadius.LG}
marginTop={2}
marginBottom={2}
paddingLeft={2}
paddingRight={copyEnabled ? 5 : 2}
color={TEXT_COLORS[variant] as TextColor}
style={{
overflowWrap: OverflowWrap.Anywhere,
minHeight: '24px',
position: 'relative',
...style,
}}
>
{copyEnabled && <CopyIcon copyText={copyText ?? ''} />}
collapsed,
}) => {
const [expanded, setExpanded] = useState(!collapsed);

const isCollapsible = collapsed !== undefined;

return (
<ConfirmInfoRowContext.Provider value={{ variant }}>
<Box
data-testid={dataTestId}
className="confirm-info-row"
display={Display.Flex}
flexDirection={FlexDirection.Row}
justifyContent={JustifyContent.center}
alignItems={AlignItems.flexStart}
color={color}
flexDirection={isCollapsible ? FlexDirection.Column : FlexDirection.Row}
justifyContent={JustifyContent.spaceBetween}
flexWrap={FlexWrap.Wrap}
alignItems={isCollapsible ? AlignItems.flexStart : AlignItems.center}
backgroundColor={BACKGROUND_COLORS[variant]}
borderRadius={BorderRadius.LG}
marginTop={2}
marginBottom={2}
paddingLeft={2}
paddingRight={copyEnabled ? 5 : 2}
color={TEXT_COLORS[variant] as TextColor}
style={{
overflowWrap: OverflowWrap.Anywhere,
minHeight: '24px',
position: 'relative',
...style,
}}
>
<Box display={Display.Flex} alignItems={AlignItems.center}>
<Text variant={TextVariant.bodyMdMedium} color={TextColor.inherit}>
{label}
</Text>
{labelChildren}
{!labelChildren && tooltip?.length && (
<Tooltip
position="bottom"
title={tooltip}
style={{ display: 'flex' }}
>
<Icon
name={TOOLTIP_ICONS[variant]}
marginLeft={1}
color={TOOLTIP_ICON_COLORS[variant] as unknown as IconColor}
size={IconSize.Sm}
{...(dataTestId
? { 'data-testid': `${dataTestId}-tooltip` }
: {})}
/>
</Tooltip>
)}
{copyEnabled && (
<CopyIcon
copyText={copyText ?? ''}
style={{ right: isCollapsible ? 32 : 0, top: 4 }}
color={IconColor.iconMuted}
/>
)}
{isCollapsible && (
<Icon
color={IconColor.iconMuted}
name={expanded ? IconName.Collapse : IconName.Expand}
size={IconSize.Sm}
style={{
cursor: 'pointer',
position: 'absolute',
right: 8,
top: 4,
}}
onClick={() => setExpanded(!expanded)}
data-testid="sectionCollapseButton"
/>
)}
<Box
display={Display.Flex}
flexDirection={FlexDirection.Row}
justifyContent={JustifyContent.center}
alignItems={AlignItems.flexStart}
color={color}
>
<Box display={Display.Flex} alignItems={AlignItems.center}>
Copy link
Member

@matthewwalsh0 matthewwalsh0 Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Assuming this behaviour is different to the expandable row, while it's nice to add configuration to the core row, this adds a lot of logic and complexity, so would it be better to create an alternate component that potentially inherits the same properties of row? Or a wrapper component?

Or at the very least, should we break this very large schema up with local components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes are required in info row requires to support collapsible row, it is hard to only wrap into row and make it collapsible.

Copy link
Member

Choose a reason for hiding this comment

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

In that case, could we split this for readability into some local components (in this file) such as Container, CopyIcon, Tooltip, Content, CollapsibleIcon etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, let me create a followup PR to do that, and avoid in this PR to change the scope here.

<Text variant={TextVariant.bodyMdMedium} color={TextColor.inherit}>
{label}
</Text>
{labelChildren}
{!labelChildren && tooltip?.length && (
<Tooltip
position="bottom"
title={tooltip}
style={{ display: 'flex' }}
>
<Icon
name={TOOLTIP_ICONS[variant]}
marginLeft={1}
color={TOOLTIP_ICON_COLORS[variant] as unknown as IconColor}
size={IconSize.Sm}
{...(dataTestId
? { 'data-testid': `${dataTestId}-tooltip` }
: {})}
/>
</Tooltip>
)}
</Box>
</Box>
{expanded &&
(typeof children === 'string' ? (
<Text marginRight={copyEnabled ? 3 : 0} color={TextColor.inherit}>
{children}
</Text>
) : (
children
))}
</Box>
{typeof children === 'string' ? (
<Text marginRight={copyEnabled ? 3 : 0} color={TextColor.inherit}>
{children}
</Text>
) : (
children
)}
</Box>
</ConfirmInfoRowContext.Provider>
);
</ConfirmInfoRowContext.Provider>
);
};
1 change: 1 addition & 0 deletions ui/components/component-library/icon/icon.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export enum IconName {
Book = 'book',
Bookmark = 'bookmark',
Bridge = 'bridge',
Collapse = 'collapse',
Calculator = 'calculator',
CardPos = 'card-pos',
CardToken = 'card-token',
Expand Down
22 changes: 20 additions & 2 deletions ui/pages/confirmations/components/confirm/info/__snapshots__/info.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,9 +554,18 @@ exports[`Info renders info section for personal sign request 1`] = `
class="mm-box mm-box--margin-bottom-4 mm-box--padding-2 mm-box--background-color-background-default mm-box--rounded-md"
>
<div
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-2 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-row mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-center mm-box--color-text-default mm-box--rounded-lg"
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-5 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-column mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-flex-start mm-box--color-text-default mm-box--rounded-lg"
style="overflow-wrap: anywhere; min-height: 24px; position: relative; background: transparent;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 32px; top: 4px;"
/>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
data-testid="sectionCollapseButton"
style="mask-image: url('./images/icons/collapse.svg'); cursor: pointer; position: absolute; right: 8px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start mm-box--color-text-default"
>
Expand Down Expand Up @@ -979,9 +988,18 @@ exports[`Info renders info section for typed sign request 1`] = `
class="mm-box mm-box--margin-bottom-4 mm-box--padding-2 mm-box--background-color-background-default mm-box--rounded-md"
>
<div
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-2 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-row mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-center mm-box--color-text-default mm-box--rounded-lg"
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-5 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-column mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-flex-start mm-box--color-text-default mm-box--rounded-lg"
style="overflow-wrap: anywhere; min-height: 24px; position: relative;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 32px; top: 4px;"
/>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
data-testid="sectionCollapseButton"
style="mask-image: url('./images/icons/collapse.svg'); cursor: pointer; position: absolute; right: 8px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start"
>
Expand Down
4 changes: 2 additions & 2 deletions ui/pages/confirmations/components/confirm/info/approve/__snapshots__/approve.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,8 +650,8 @@ exports[`<ApproveInfo /> renders component for approve request 1`] = `
style="overflow-wrap: anywhere; min-height: 24px; position: relative;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-alternative"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 0px; top: 2px;"
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 0px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start"
Expand Down
22 changes: 20 additions & 2 deletions ...irmations/components/confirm/info/personal-sign/__snapshots__/personal-sign.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,18 @@ exports[`PersonalSignInfo handle reverse string properly 1`] = `
class="mm-box mm-box--margin-bottom-4 mm-box--padding-2 mm-box--background-color-background-default mm-box--rounded-md"
>
<div
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-2 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-row mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-center mm-box--color-text-default mm-box--rounded-lg"
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-5 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-column mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-flex-start mm-box--color-text-default mm-box--rounded-lg"
style="overflow-wrap: anywhere; min-height: 24px; position: relative; background: transparent;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 32px; top: 4px;"
/>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
data-testid="sectionCollapseButton"
style="mask-image: url('./images/icons/collapse.svg'); cursor: pointer; position: absolute; right: 8px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start mm-box--color-text-default"
>
Expand Down Expand Up @@ -135,9 +144,18 @@ exports[`PersonalSignInfo renders correctly for personal sign request 1`] = `
class="mm-box mm-box--margin-bottom-4 mm-box--padding-2 mm-box--background-color-background-default mm-box--rounded-md"
>
<div
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-2 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-row mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-center mm-box--color-text-default mm-box--rounded-lg"
class="mm-box confirm-info-row mm-box--margin-top-2 mm-box--margin-bottom-2 mm-box--padding-right-5 mm-box--padding-left-2 mm-box--display-flex mm-box--flex-direction-column mm-box--flex-wrap-wrap mm-box--justify-content-space-between mm-box--align-items-flex-start mm-box--color-text-default mm-box--rounded-lg"
style="overflow-wrap: anywhere; min-height: 24px; position: relative; background: transparent;"
>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
style="mask-image: url('./images/icons/copy.svg'); cursor: pointer; position: absolute; right: 32px; top: 4px;"
/>
<span
class="mm-box mm-icon mm-icon--size-sm mm-box--display-inline-block mm-box--color-icon-muted"
data-testid="sectionCollapseButton"
style="mask-image: url('./images/icons/collapse.svg'); cursor: pointer; position: absolute; right: 8px; top: 4px;"
/>
<div
class="mm-box mm-box--display-flex mm-box--flex-direction-row mm-box--justify-content-center mm-box--align-items-flex-start mm-box--color-text-default"
>
Expand Down
Loading