Component cleanup: Icons, paths, links, code style (3928) #3319
Conversation
some rules suppressed to avoid changing implementation
| @@ -36,6 +36,7 @@ const PricingTitleBadge = ( { item } ) => { | |||
| ); | |||
|
|
|||
| const label = sprintf( | |||
| // Translators: %1$s is the percentage, %2$s is the fixed amount. | |||
| @@ -57,7 +57,9 @@ const filterSteps = ( steps, conditions ) => { | |||
| }; | |||
|
|
|||
| export const getSteps = ( flags ) => { | |||
| // eslint-disable-next-line react-hooks/rules-of-hooks | |||
There was a problem hiding this comment.
Instead of ignoring the "rules-of-hooks" check:
- My initial idea was to pass all conditions as arguments, like
getSteps( flags, skipPaymentsScreen ), ie keep it a plain helper function without dependencies (would also make it more testable) - An alternative would be to change the helper method to become a real hook -- change
getSteps(flags)touseSteps()or similar, then we can drop the flags argument and collect all details inside the hook
The current mix of receiving input arguments and collecting other details via hooks is a bit messy
| ? // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| CommonHooks.useSandbox() | ||
| : // eslint-disable-next-line react-hooks/rules-of-hooks | ||
| CommonHooks.useProduction(); |
There was a problem hiding this comment.
Instead of ignoring these warnings, I think it would be better to correct the code to call hooks unconditionally:
const sandboxData = CommonHooks.useSandbox();
const productionData = CommonHooks.useProduction();
const { onboardingUrl } = isSandbox ? sandboxData : productionData;| const [ syncCompleted, setSyncCompleted ] = useState( false ); | ||
| const [ syncError, setSyncError ] = useState( null ); | ||
| const [ , setSyncCompleted ] = useState( false ); | ||
| const [ , setSyncError ] = useState( null ); |
There was a problem hiding this comment.
Hm... looks like we do not use those two states. There's no point in setting the state but not using the value anywhere.
To clean this up, the two states should be completely removed from the code.
Possibly, those states are a left over from a previous change
|
As we're working on this, can you also remove this utility file? This file contains a deprecated helper that's only used in a single component: I would really like to delete data.js, as we should not use the getImage() helper anywhere, except inside the PaymentMethodIcon component - here's an idea for a much cleaner solution: import { Icon } from '@wordpress/components';
const imageUrl( name ) {
const filename = `icon-button-${ name }.svg`;
// Move the logic into this file, as a private helper method.
}
const PaymentMethodIcon = ( { type } ) => (
<Icon
icon={ imageUrl( type ) }
className="ppcp--method-icon"
/>
);
export default PaymentMethodIcon; |
This PR fixes js lint in the settings module. Some rules were suppressed to avoid changing implementation