Remove double modal #468
Remove double modal #468
Conversation
WalkthroughWalkthroughThe updates to the application significantly enhance the user interface and functionality of wallet and federation management components. Key improvements include a streamlined tabbed interface for currency unit selection, enhanced error handling with dedicated components, and the introduction of new features for managing deposits and withdrawals. Collectively, these changes improve usability, maintainability, and user experience, making interactions more intuitive and efficient. Changes
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
| @@ -151,15 +108,15 @@ export const ConnectFederation = React.memo(function ConnectFederation({ | |||
| /> | |||
| <Button | |||
| borderRadius='8px' | |||
| maxW='210px' | |||
There was a problem hiding this comment.
Note for reviewers: We're removing this so that the loadingText below isn't cut off
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -158,7 +158,7 @@ | |||
| "current-session": "Current Session", | |||
| "session-to-shutdown": "Session to Shutdown at", | |||
| "confirm-shutdown": "Confirm Shutdown", | |||
| "session-to-shutdown-helper": "Enter the session height at which you want to shutdown your guardian node." | |||
| "session-to-shutdown-helper": "." | |||
There was a problem hiding this comment.
Retain the descriptive text for clarity.
The change to "session-to-shutdown-helper" from a descriptive message to a single period removes valuable guidance for users. This could negatively impact user experience by leaving them without context or instructions regarding the session shutdown process. Consider retaining the original descriptive text.
- "session-to-shutdown-helper": "."
+ "session-to-shutdown-helper": "Enter the session height at which you want to shutdown your guardian node."Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "session-to-shutdown-helper": "." | |
| "session-to-shutdown-helper": "Enter the session height at which you want to shutdown your guardian node." |
| import React from 'react'; | ||
| import { Flex, Heading, Text, useTheme } from '@chakra-ui/react'; | ||
| import { useTranslation } from '@fedimint/utils'; | ||
|
|
||
| interface ErrorProps { | ||
| error: string; | ||
| } | ||
|
|
||
| export const Error: React.FC<ErrorProps> = ({ error }) => { | ||
| const { t } = useTranslation(); | ||
| const theme = useTheme(); | ||
|
|
||
| return ( | ||
| <Flex | ||
| direction='column' | ||
| align='center' | ||
| width='100%' | ||
| paddingTop='10vh' | ||
| paddingX='4' | ||
| textAlign='center' | ||
| > | ||
| <Heading size='lg' marginBottom='4' color={theme.colors.red[500]}> | ||
| {t('common.error')} | ||
| </Heading> | ||
| <Text fontSize='md' color={theme.colors.gray[700]}> | ||
| {error} | ||
| </Text> | ||
| </Flex> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Rename the Error component to avoid shadowing the global Error object.
The component name Error shadows the global Error object, which can lead to confusion and potential issues. Consider renaming the component to ErrorMessage.
- export const Error: React.FC<ErrorProps> = ({ error }) => {
+ export const ErrorMessage: React.FC<ErrorProps> = ({ error }) => {Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import React from 'react'; | |
| import { Flex, Heading, Text, useTheme } from '@chakra-ui/react'; | |
| import { useTranslation } from '@fedimint/utils'; | |
| interface ErrorProps { | |
| error: string; | |
| } | |
| export const Error: React.FC<ErrorProps> = ({ error }) => { | |
| const { t } = useTranslation(); | |
| const theme = useTheme(); | |
| return ( | |
| <Flex | |
| direction='column' | |
| align='center' | |
| width='100%' | |
| paddingTop='10vh' | |
| paddingX='4' | |
| textAlign='center' | |
| > | |
| <Heading size='lg' marginBottom='4' color={theme.colors.red[500]}> | |
| {t('common.error')} | |
| </Heading> | |
| <Text fontSize='md' color={theme.colors.gray[700]}> | |
| {error} | |
| </Text> | |
| </Flex> | |
| ); | |
| }; | |
| import React from 'react'; | |
| import { Flex, Heading, Text, useTheme } from '@chakra-ui/react'; | |
| import { useTranslation } from '@fedimint/utils'; | |
| interface ErrorProps { | |
| error: string; | |
| } | |
| export const ErrorMessage: React.FC<ErrorProps> = ({ error }) => { | |
| const { t } = useTranslation(); | |
| const theme = useTheme(); | |
| return ( | |
| <Flex | |
| direction='column' | |
| align='center' | |
| width='100%' | |
| paddingTop='10vh' | |
| paddingX='4' | |
| textAlign='center' | |
| > | |
| <Heading size='lg' marginBottom='4' color={theme.colors.red[500]}> | |
| {t('common.error')} | |
| </Heading> | |
| <Text fontSize='md' color={theme.colors.gray[700]}> | |
| {error} | |
| </Text> | |
| </Flex> | |
| ); | |
| }; |
Tools
Biome
[error] 9-9: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| import { FederationsTable } from './components/federations/FederationsTable'; | ||
| import { Loading } from './components/Loading'; | ||
| import { Error } from './components/Error'; |
There was a problem hiding this comment.
Shadowing global "Error" property.
Consider renaming the Error component to avoid shadowing the global Error property.
- import { Error } from './components/Error';
+ import { ErrorComponent } from './components/Error';Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { FederationsTable } from './components/federations/FederationsTable'; | |
| import { Loading } from './components/Loading'; | |
| import { Error } from './components/Error'; | |
| import { FederationsTable } from './components/federations/FederationsTable'; | |
| import { Loading } from './components/Loading'; | |
| import { ErrorComponent } from './components/Error'; |
Tools
Biome
[error] 19-19: Do not shadow the global "Error" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| [network] | ||
| ); | ||
|
|
||
| const url = useMemo(() => getAddressUrl(address), [address, getAddressUrl]); |
There was a problem hiding this comment.
Unused URL variable.
The url variable is assigned a value but never used. Consider removing it if not needed.
Tools
GitHub Check: Lint
[warning] 93-93:
'url' is assigned a value but never used
| const getAddressUrl = useCallback( | ||
| (address: string): URL => { | ||
| const baseAddress = address.split(':')[1]?.split('?')[0] || ''; | ||
| switch (network) { | ||
| case Network.Signet: | ||
| return new URL(`https://mutinynet.com/address/${baseAddress}`); | ||
| case Network.Testnet: | ||
| return new URL( | ||
| `https://mempool.space/testnet/address/${baseAddress}` | ||
| ); | ||
| case Network.Bitcoin: | ||
| case Network.Regtest: | ||
| default: | ||
| return new URL(`https://mempool.space/address/${baseAddress}`); | ||
| } | ||
| }, | ||
| [network] | ||
| ); |
There was a problem hiding this comment.
Get address URL logic.
The getAddressUrl function is correctly implemented but contains redundant case clauses.
- case Network.Bitcoin:
- case Network.Regtest:Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const getAddressUrl = useCallback( | |
| (address: string): URL => { | |
| const baseAddress = address.split(':')[1]?.split('?')[0] || ''; | |
| switch (network) { | |
| case Network.Signet: | |
| return new URL(`https://mutinynet.com/address/${baseAddress}`); | |
| case Network.Testnet: | |
| return new URL( | |
| `https://mempool.space/testnet/address/${baseAddress}` | |
| ); | |
| case Network.Bitcoin: | |
| case Network.Regtest: | |
| default: | |
| return new URL(`https://mempool.space/address/${baseAddress}`); | |
| } | |
| }, | |
| [network] | |
| ); | |
| const getAddressUrl = useCallback( | |
| (address: string): URL => { | |
| const baseAddress = address.split(':')[1]?.split('?')[0] || ''; | |
| switch (network) { | |
| case Network.Signet: | |
| return new URL(`https://mutinynet.com/address/${baseAddress}`); | |
| case Network.Testnet: | |
| return new URL( | |
| `https://mempool.space/testnet/address/${baseAddress}` | |
| ); | |
| default: | |
| return new URL(`https://mempool.space/address/${baseAddress}`); | |
| } | |
| }, | |
| [network] | |
| ); |
Tools
Biome
[error] 84-84: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
[error] 85-85: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
| import React, { useState, FC } from 'react'; | ||
| import { | ||
| Box, | ||
| Button, | ||
| Heading, | ||
| Stack, | ||
| Text, | ||
| useTheme, | ||
| Flex, | ||
| Textarea, | ||
| Modal, | ||
| ModalBody, | ||
| ModalOverlay, | ||
| ModalContent, | ||
| CircularProgressLabel, | ||
| CircularProgress, | ||
| ModalCloseButton, | ||
| } from '@chakra-ui/react'; |
There was a problem hiding this comment.
Remove unused imports.
The following imports are unused and should be removed: FC, Flex, CircularProgressLabel, CircularProgress.
- import React, { useState, FC } from 'react';
+ import React, { useState } from 'react';
- import { Flex, CircularProgressLabel, CircularProgress } from '@chakra-ui/react';
+ import { CircularProgressLabel, CircularProgress } from '@chakra-ui/react';Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Lint
[warning] 1-1:
'FC' is defined but never used
[warning] 9-9:
'Flex' is defined but never used
[warning] 15-15:
'CircularProgressLabel' is defined but never used
[warning] 16-16:
'CircularProgress' is defined but never used
tvolk131
force-pushed
the
remove_double_modal
branch
from
1fed174
to
9ac38f4
Compare
|
Closing in favor of #470 since it's not stacked on another PR |
Requires #455
When connecting to a new federation, show a loading spinner on the "Add Federation" button rather than showing a second modal with a loading spinner on top of the first modal
Summary by CodeRabbit
New Features
FederationsTablefor managing deposits and withdrawals, enhancing usability.ConnectFederationcomponent to facilitate federation connections with a user-friendly modal interface.WalletCardfor managing cryptocurrency wallet operations, including deposits and withdrawals.Bug Fixes
Documentation
Chores
Style