fix(suite): restructure SettingsSection to properly display explanato… #16957
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
vojtatranta
force-pushed
the
16610-fix-text-breaking-coins
branch
from
c770f64 to
528e0e6
Compare
|
QA instruct: please, test out other languages / windows / all of the other settings sections if the styling is correct and texts are readable and tooltips are displayed properly with correct texts, thanks |
| @@ -44,6 +44,7 @@ const Container = styled.div<ContainerProps>` | |||
|
|
|||
| export type InfoItemProps = AllowedFrameProps & | |||
| AllowedTextProps & { | |||
| allowTextWrap?: boolean; | |||
There was a problem hiding this comment.
What about do it with textProps using ellipsisLineCount? It's the same principle like frameProps
Check it here: packages/components/src/components/typography/utils.tsx
There was a problem hiding this comment.
The it woul be liie ellipsisLineCOunt={0}, right?
There was a problem hiding this comment.
Why 0? I think it's correct this way: ellipsisLineCount ?? 1
There was a problem hiding this comment.
As we discussed: you need to pass then to this compoennt ellipsisLineCount={0} to disable line breaking (the inner comopnent has check ellipsisLineCount > 0 to enable text overflow and ellipsis.
vojtatranta
force-pushed
the
16610-fix-text-breaking-coins
branch
from
528e0e6 to
4beba2e
Compare
|
QA OK Screen.Recording.2025-02-13.at.09.53.53.movInfo:
|
vojtatranta
force-pushed
the
16610-fix-text-breaking-coins
branch
from
4beba2e to
e2ece74
Compare
WalkthroughThis pull request implements modifications to three sections of the codebase. In the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/suite/src/components/settings/SettingsSection.tsx (1)
33-51: Consider extracting tooltip rendering to a separate component.The tooltip rendering logic could be moved to a dedicated component to improve maintainability and reusability.
+const TooltipIcon = ({ content }: { content: ReactNode }) => ( + <Box + as="span" + position={{ + type: 'relative', + top: 2, + left: 5, + }} + > + <Tooltip isInline content={content}> + <Icon name="question" size="medium" /> + </Tooltip> + </Box> +); export const SettingsSection = ({ title, icon, children, tooltipText }: SettingsSectionProps) => { // ... label={ <> {title} - {tooltipText && ( - <Box - as="span" - position={{ - type: 'relative', - top: 2, - left: 5, - }} - > - <Tooltip isInline content={tooltipText}> - <Icon name="question" size="medium" /> - </Tooltip> - </Box> - )} + {tooltipText && <TooltipIcon content={tooltipText} />} </> } // ...packages/components/src/components/InfoItem/InfoItem.tsx (1)
50-50: Consider documenting the allowTextWrap prop.The allowTextWrap prop lacks documentation explaining its purpose and usage.
export type InfoItemProps = AllowedFrameProps & AllowedTextProps & { + /** Controls whether text should wrap to multiple lines */ allowTextWrap?: boolean;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/src/components/InfoItem/InfoItem.tsx(4 hunks)packages/suite/src/components/settings/SettingsSection.tsx(1 hunks)packages/suite/src/views/settings/SettingsCoins/SettingsCoins.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: run-desktop-tests (@group=wallet, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=other, trezor-user-env-unix)
- GitHub Check: run-desktop-tests (@group=settings, trezor-user-env-unix bitcoin-regtest)
- GitHub Check: run-desktop-tests (@group=device-management, trezor-user-env-unix)
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: build-web
- GitHub Check: run-desktop-tests (@group=suite, trezor-user-env-unix)
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (4)
packages/suite/src/components/settings/SettingsSection.tsx (1)
21-21: LGTM! New tooltipText prop added.The addition of the optional tooltipText prop enhances the component's flexibility for displaying explanatory tooltips.
packages/components/src/components/InfoItem/InfoItem.tsx (2)
26-29: LGTM! Added ellipsisLineCount to allowed text props.The addition of ellipsisLineCount to allowedInfoItemTextProps enables better control over text truncation.
102-102: LGTM! Default ellipsisLineCount implementation.The nullish coalescing operator provides a sensible default of 1 while allowing explicit override.
packages/suite/src/views/settings/SettingsCoins/SettingsCoins.tsx (1)
130-144: LGTM! Consistent usage of tooltipText prop.The implementation correctly uses the new tooltipText prop across all SettingsSection components, improving code consistency and readability.
vojtatranta
force-pushed
the
16610-fix-text-breaking-coins
branch
2 times, most recently
from
7e9ba2d to
9007adf
Compare
vojtatranta
force-pushed
the
16610-fix-text-breaking-coins
branch
from
9007adf to
58057ef
Compare
…ry tooltips
TODO:
Description
Related Issue
Resolve #16610
Screenshots:
BRF
AFTR
