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.

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

feat(suite): better Firmware changelog #13354

Merged
merged 2 commits into from
Jul 19, 2024
Merged

feat(suite): better Firmware changelog #13354

merged 2 commits into from
Jul 19, 2024

Conversation

peter-sanderson
Copy link
Contributor

@peter-sanderson peter-sanderson commented Jul 15, 2024

Resolves: #12783
Resolves: #6987

Change-log now can be written as markdown and lines can be separated for better readability:
image

Before:
image

After:
image

@peter-sanderson peter-sanderson force-pushed the changelog branch 3 times, most recently from fee5c9c to 13c012b Compare July 16, 2024 14:08
@peter-sanderson peter-sanderson marked this pull request as ready for review July 16, 2024 14:23
@peter-sanderson peter-sanderson force-pushed the changelog branch 4 times, most recently from 9859412 to 40ef624 Compare July 16, 2024 14:27
@peter-sanderson peter-sanderson requested review from komret and removed request for szymonlesisz, martykan and mroz22 July 16, 2024 14:27
@jvaclavik
Copy link
Contributor

First impression:

View all + icon is a bit ugly, could you polish it please? 🙏

If possible I would recommend:

  • add some gap between icon and test
  • disable underline
  • primary/secondary color

Copy link
Contributor

@komret komret left a comment

Choose a reason for hiding this comment

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

Please check if the new Markdown can be used in the Available component as well.

Screenshot 2024-07-17 at 15 05 21

@@ -71,9 +70,12 @@ export const FirmwareOffer = ({ customFirmware, targetFirmwareType }: FirmwareOf
const nextVersion = customFirmware
? translationString('TR_CUSTOM_FIRMWARE_VERSION')
: getFwUpdateVersion(originalDevice);

const isBtcOnly = originalDevice.firmwareType === FirmwareType.BitcoinOnly;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to take targetFirmwareType into account to work properly in firmware type switch flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a device here to test with, but this probably won't work correctly during FW installation when switching type. targetFirmwareType from props is used in case installation hasn't started yet, while targetType is set when installation begins and used in case targetFirmwareType prop is not provided. Please test it with e5ebf0e.

@peter-sanderson
Copy link
Contributor Author

Available

image

@peter-sanderson peter-sanderson force-pushed the changelog branch 2 times, most recently from 6a1479f to e240c63 Compare July 18, 2024 09:41
}
`;

export const Markdown = (options: Readonly<Options>) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also add stories for this component? 🙏
I'd like to keep stories for each DS component. Otherwise it's hard to discover

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

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

please please connect related stuff into a single commit 🙏

@peter-sanderson
Copy link
Contributor Author

@komret

BTC only -> Universal shows correct change-log:
image

Universal -> BTC only shows correct changelog
image

@peter-sanderson peter-sanderson force-pushed the changelog branch 2 times, most recently from 83fe8dc to 2c4dde9 Compare July 18, 2024 13:14
Copy link

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…pport for all changelogs
@peter-sanderson peter-sanderson merged commit a707883 into develop Jul 19, 2024
86 checks passed
@peter-sanderson peter-sanderson deleted the changelog branch July 19, 2024 07:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve FW changelog in Suite Distinguish Bitcoin-only FW changelog
4 participants