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

chore(suite): interface cleanup for TroubleshootingTips #17972

Merged
merged 1 commit into from
Mar 31, 2025

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Mar 28, 2025

Description

Ex post CR suggestions for #17925

Mainly refactor props, IMO more intuitive now and without too many diffs.

And small nits

Screenshots:

Proof that I did not mess up the order of the props:
BT on
bt
BT off
without bt
What we've got here is failure to communicate
failed comm

@coderabbitai ignore

@Lemonexe Lemonexe added the no-project This label is used to specify that PR doesn't need to be added to a project label Mar 28, 2025
@Lemonexe Lemonexe marked this pull request as ready for review March 28, 2025 20:25
Copy link

🔍🌐 Suite Web test results: View in Currents

Copy link

🔍🖥️ Suite Desktop test results: View in Currents

@@ -81,15 +81,15 @@ export const DeviceConnect = ({ setIsBluetoothConnectOpen }: DeviceConnectProps)

return (
<TroubleshootingTipsWithSections
label={<Translation id="TR_TREZOR_SAFE_7" />}
label={<Translation id="TR_CONNECTION_TYPE" />}
ctaLabel={<Translation id="TR_TREZOR_SAFE_7" />}
Copy link
Contributor Author

@Lemonexe Lemonexe Mar 28, 2025

Choose a reason for hiding this comment

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

before → after:

  • sectionLabellabel
  • labelctaLabel

label is now the same thing for TroubleshootingTipsWithSections and for TroubleshootingTips, which was the most confusing thing for me personally..
Visually it is the most prominent label, as it headlines the table, so it makes sense that the name reflects that this is the label.

ctaLabel now actually behaves analogically to toggleText – it is just custom override, and if not supplied, a default will be used (label for cta, constant for toggle).

Here is currently the only usage of the ctaLabel override...

const [selectedSection, setSelectedSection] = useState<K>(defaultSection);
const [isOpened, setIsOpened] = useState(initiallyIsOpen === true);
const firstSectionKey = Object.keys(items)[0] as K;
const [selectedSection, setSelectedSection] = useState<K>(defaultSection ?? firstSectionKey);
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

@Lemonexe Lemonexe force-pushed the bluetooth-ready-wellcome-screen-CR-suggestion branch from 018d365 to e2a5b9d Compare March 31, 2025 08:35
@Lemonexe Lemonexe merged commit f028420 into develop Mar 31, 2025
34 checks passed
@Lemonexe Lemonexe deleted the bluetooth-ready-wellcome-screen-CR-suggestion branch March 31, 2025 09:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-project This label is used to specify that PR doesn't need to be added to a project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants