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

Update icon quick actions #14077

Merged
merged 1 commit into from
Sep 7, 2024
Merged

Update icon quick actions #14077

merged 1 commit into from
Sep 7, 2024

Conversation

peter-sanderson
Copy link
Contributor

@peter-sanderson peter-sanderson commented Sep 2, 2024 •
edited
Loading

Followup of #14014, part of #13990

Todos / Known Issues:

  • Fix horizontal scrollbar
  • Enhance centering of USB cable

This PR:

  1. Adds a Update Device & Suite Icons into QuickActions Bar
  2. Adds a tooltip to it
  3. Fixes some minor color issues in other QuickActions Icons

image
image
image

@peter-sanderson peter-sanderson force-pushed the update-icon-quick-actions branch 7 times, most recently from 30673bd to 4a2e0cc Compare September 4, 2024 23:10
@peter-sanderson peter-sanderson force-pushed the update-icon-quick-actions branch 2 times, most recently from 0c7cc3b to b076abe Compare September 5, 2024 21:31
@peter-sanderson peter-sanderson marked this pull request as ready for review September 5, 2024 21:33
peter-sanderson
peter-sanderson commented Sep 5, 2024
View reviewed changes
...uite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/useUpdateStatus.ts Outdated

const common: Omit<UpdateStatusData, 'updateStatus'> = {
updateStatusDevice: isFirmwareOutdated ? 'update-available' : 'up-to-date',
// eslint-disable-next-line no-nested-ternary
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo: once logic is settled (after product's feedback)

...c/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/updateQuickActionTypes.ts Outdated
return theme[colorMap[$variant]];
};

export const mapTrezorModelToIcon: Record<DeviceModelInternal, IconName> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jvaclavik This shall be at some shared places? What do you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to move it here:

packages/product-components/src/utils/mapTrezorModelToIcon.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

jvaclavik
jvaclavik approved these changes Sep 6, 2024
View reviewed changes
Copy link
Contributor

@jvaclavik jvaclavik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good job! ship it 🚀

Just minor comments.

packages/suite-desktop-api/src/messages.ts Show resolved Hide resolved
packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/QuickActions.tsx
padding: ${spacingsPx.xxs};
gap: ${spacingsPx.xs};

border-top: 1px solid ${({ theme }) => theme.borderElevation1};
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add elevated border here? 🙏

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? Can you share some CSS? 🙏 (dunno what "elevated border" exactly means)

packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Tor.tsx Outdated
@@ -48,7 +48,7 @@ export const Tor = () => {
size: iconSizes.extraSmall,
}}
>
<Icon name="tor" size={iconSizes.medium} />
<Icon name="tor" size={iconSizes.medium} variant="tertiary" />
Copy link
Contributor

Choose a reason for hiding this comment

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

please could you update it to name="torBrowser"? 🙏 tor is legacy icon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/Update.tsx Outdated
);
};

export const Update = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename it to something more descriptive or with some noun? I think it's easier when you are looking for component in your IDE.
Not sure but thinking about something like ManageUpdate or UpdateStatus

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/Update.tsx Outdated
$size: IconSize;
};

const UsbCableFroTrezorIcon = styled.div<UsbCableFroTrezorIcon>`
Copy link
Contributor

Choose a reason for hiding this comment

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

typo in component name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

packages/suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/Update.tsx Outdated
return null;
}

const handleOnclick = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

should be handleOnClick (you have lower case in C)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

...suite/src/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/UpdateTooltip.tsx Outdated

type UpdateRowProps = {
children: ReactNode;
leftIcon: ReactNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

How about using type IconName like everywhere else in case of icons? And if you want to add different component you could rename it to leftItemComponent. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I renamed it to leftItem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

80d4082

...c/components/suite/layouts/SuiteLayout/Sidebar/QuickActions/Update/updateQuickActionTypes.ts Outdated
return theme[colorMap[$variant]];
};

export const mapTrezorModelToIcon: Record<DeviceModelInternal, IconName> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest to move it here:

packages/product-components/src/utils/mapTrezorModelToIcon.ts

@peter-sanderson peter-sanderson force-pushed the update-icon-quick-actions branch 2 times, most recently from 9d043ac to 80d4082 Compare September 6, 2024 17:09
@peter-sanderson peter-sanderson merged commit 27a6c63 into develop Sep 7, 2024
24 checks passed
@peter-sanderson peter-sanderson deleted the update-icon-quick-actions branch September 7, 2024 12:08
@bosomt
Copy link
Contributor

bosomt commented Oct 1, 2024

QA OK

Info:

  • Suite version: desktop 24.10.0 (3e1d4df)
  • Browser: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) TrezorSuiteDev/24.10.0 Chrome/126.0.6478.234 Electron/31.6.0 Safari/537.36
  • OS: MacIntel
  • Screen: 1512x982
  • Device: Trezor T2T1 2.8.1 regular (revision 632b9561559b7ab6824bb7eeac072874e07b7b82)
  • Transport: BridgeTransport 3.0.0-bundled.24.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvaclavik jvaclavik jvaclavik approved these changes

@MiroslavProchazka MiroslavProchazka Awaiting requested review from MiroslavProchazka MiroslavProchazka is a code owner

@Nodonisko Nodonisko Awaiting requested review from Nodonisko Nodonisko is a code owner

@adamhavel adamhavel Awaiting requested review from adamhavel adamhavel is a code owner

@szymonlesisz szymonlesisz Awaiting requested review from szymonlesisz szymonlesisz is a code owner

@marekrjpolak marekrjpolak Awaiting requested review from marekrjpolak marekrjpolak is a code owner

Assignees
No one assigned
Labels
None yet
Projects
Issues Suite
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peter-sanderson @bosomt @jvaclavik