feat(suite): refactor advanced coin settings modal #17434
Conversation
seibei-iguchi
force-pushed
the
feat/refactor-advanced-settings-modal
branch
3 times, most recently
from
589c81f to
caf1784
Compare
seibei-iguchi
force-pushed
the
feat/refactor-advanced-settings-modal
branch
from
caf1784 to
869bc6c
Compare
WalkthroughThe pull request introduces several adjustments across multiple components. In the components package, a new optional prop for test identification is added to a List item while modal and typography components receive updates for conditional styling and additional text casing options. The accompanying utilities have been updated to support the new text casing properties, including the definition of allowed cases. In the suite package, the Advanced Coin Settings modal undergoes significant restructuring with a shift to a component-based approach using updated modal components. Several components related to backend settings are removed or refactored, including updates to backend URL management and interaction with Tor settings. Additionally, a new translation message is introduced, and the submodule reference for a common dependency is updated to a newer commit. All changes are purely additive or refactoring, without affecting existing functionality. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/ConnectionInfo.tsx (1)
17-34: Avoid using<br />tags for spacing.
Repeated<br />tags can lead to inconsistent layout, especially if theming or translations change lengths. Prefer separate<Paragraph>components or styling with spacing tokens.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/AdvancedCoinSettingsModal.tsx (1)
86-197: Consider reducing component complexity.
The large rendering block shows multiple UI elements: arrangement within aCard,Column,List,CollapsibleBox, and more. Extracting subcomponents (e.g., for the URL list) would improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/components/src/components/List/ListItem.tsx(1 hunks)packages/components/src/components/NewModal/NewModal.tsx(1 hunks)packages/components/src/components/typography/Text/Text.tsx(1 hunks)packages/components/src/components/typography/utils.tsx(5 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/AdvancedCoinSettingsModal.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/BackendInput.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/BackendTypeSelect.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/ConnectionInfo.tsx(1 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/CustomBackends.tsx(0 hunks)packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/TorModal.tsx(2 hunks)packages/suite/src/support/messages.ts(1 hunks)submodules/trezor-common(1 hunks)
💤 Files with no reviewable changes (2)
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/BackendInput.tsx
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/CustomBackends.tsx
✅ Files skipped from review due to trivial changes (2)
- submodules/trezor-common
- packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/BackendTypeSelect.tsx
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Unit Tests
- GitHub Check: Releases revision Checks
- GitHub Check: Build libs for publishing
- GitHub Check: Type Checking
- GitHub Check: Linting and formatting
- GitHub Check: build-web
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (17)
packages/suite/src/support/messages.ts (1)
4514-4517: New translation entry looks good.The addition of the
TR_REMOVEtranslation entry follows the established pattern of the message definitions file and will support the refactored advanced coin settings modal mentioned in the PR objectives.packages/components/src/components/typography/Text/Text.tsx (1)
27-27: Enhances text styling options by adding case controlThe addition of 'case' to the
allowedTextTextPropsarray extends the Text component's capabilities, allowing developers to control text casing (uppercase, lowercase, titlecase) through this prop.packages/components/src/components/NewModal/NewModal.tsx (1)
107-107: Improves header alignment for better visual hierarchyThis change enhances the modal's header layout by conditionally aligning items to the top (
flex-start) when a description is present, while maintaining center alignment when there's only a heading. This creates a more natural reading flow and cleaner visual hierarchy.packages/components/src/components/List/ListItem.tsx (1)
45-45: Added test identification support to ListItemThe addition of the
data-testidproperty improves the component's testability, making it easier to select specific list items in automated tests.Also applies to: 48-52, 56-56
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/TorModal.tsx (1)
1-1: Successfully refactored TorModal to use the new modal systemThe modal has been completely refactored to use the
NewModalcomponent instead of the previous custom styled modal. This change:
- Simplifies the component structure
- Adds consistent styling with
size="small"and the Tor icon- Uses appropriate typography components (
H3,Paragraph) for better accessibility- Implements the new button pattern with
NewModal.ButtonThese changes align with the PR objective to refactor the advanced coin settings modal for a more consistent user experience.
Also applies to: 4-4, 18-44
packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/CustomBackends/ConnectionInfo.tsx (1)
7-9: Use of type alias is fine.
Good switch from interface to type alias. This is purely stylistic, and it remains compatible with advanced type compositions.packages/components/src/components/typography/utils.tsx (7)
11-13: Introduction of textCase is well-implemented.
Defining all possible case options as a const assertion with a derived type is concise and clear.
19-19: Optional 'case' property.
Addingcase?: TextCase;is a straightforward extension to support text transformations.
39-39: Adding$caseto transient props.
Forwarding$caseensures it’s available in the styled interpolation. This aligns well with your approach.
50-66: Check cross-browser support for multiline ellipses.
Ensuringtext-overflow: ellipsisand multi-line clamps are supported across all required browsers is crucial. Edge cases might need fallback logic.Would you like me to generate a script to scan usage patterns of line clamp, or do you plan to rely on a documented support matrix?
67-77: "titlecase" only capitalizes the first letter of the entire string.
If the requirement is to capitalize each word (typical “title case”), additional logic is needed. Otherwise, this only changes the first character.
110-116: Storybook integration.
Including the new “case” control is a helpful addition for testing transformations.
146-146: Default arg for 'case' prop.
Providing a default ofundefinedprevents unintentional transformations and keeps stories consistent.packages/suite/src/components/suite/modals/ReduxModal/UserContextModal/AdvancedCoinSettingsModal/AdvancedCoinSettingsModal.tsx (4)
28-31: Consistent use of type alias for props.
DeclaringAdvancedCoinSettingsModalPropsas a type is stylistically fine, matching other components’ newer patterns.
51-58: Conditional Tor enablement logic.
The check for onion URLs before enabling Tor is logical. However, consider user feedback if they have multiple onion-only backends inadvertently.
60-76: Potential consistency gap after 'use-defaults'.
When the user chooses "use-defaults", you callchangeType('default')but do not finalize or close the modal. Confirm this meets user expectations.
83-85: Modal toggling approach is valid.
Swapping the entire component content based ontorModalOpenis straightforward and clear.
| @@ -24,6 +24,7 @@ export const allowedTextTextProps = [ | |||
| 'textWrap', | |||
| 'align', | |||
| 'ellipsisLineCount', | |||
| 'case', | |||
There was a problem hiding this comment.
This prop name seems to me very confusing. Why not keep textTransform?
There was a problem hiding this comment.
I see, I know why you did it. It's because titlecase is non-standard. OK then...
There was a problem hiding this comment.
git submodule update --init --recursive
| const { isTorEnabled } = useSelector(selectTorState); | ||
| const blockchain = useSelector(state => state.wallet.blockchain); | ||
| const dispatch = useDispatch(); | ||
| const [torModalOpen, setTorModalOpen] = useState(false); |
There was a problem hiding this comment.
nitpick: maybe torModalOpened or isTorModalOpened?
Screenshots:
Before
After