refactor: [IOPLT-000] Class and HOC to React hooks#8262
Conversation
LeleDallas
added
enhancement
💀 Dead matter
PR Title Validation for conventional commit type✅ All good! PR title follows the conventional commit type. |
Jira Pull Request LinkThis Pull Request refers to Jira issues: |
There was a problem hiding this comment.
If we removed withOfflineFailureScreen, it makes sense to rename the file to useOfflineFailureScreen as well
There was a problem hiding this comment.
Since the helper has become a hook, I think it would be better to move it in that folder. What do you think?
There was a problem hiding this comment.
Since the helper has become a hook, I think it would be better to move it in that folder. What do you think?
@LeleDallas I agree 👍🏻
There was a problem hiding this comment.
Why an hook instead of a WithOfflineFailureScreen component?
Is not an HOC but offers the same DX.
if {} else {} sucks when you need to deal with hooks
There was a problem hiding this comment.
I went with a hook because the reusable part is the logic for determining the offline state and tracking the analytics event, not the UI itself.
The hook keeps those concerns separate and lets each screen decide what to render when offline. A wrapper component would couple that logic to a specific rendering strategy and make it less flexible if a screen needs different behavior in the future.
There was a problem hiding this comment.
I'm not fully convinced by this approach yet 😄
I went with a hook because the reusable part is the logic for determining the offline state and tracking the analytics event, not the UI itself.
I think the reusable part includes both the logic and the UI behaviour. In this case we are not simply determining whether the device is offline but we are deciding whether the entire screen should be replaced with a failure screen.
In other words, this is not just an useIsOffline hook.
The hook keeps those concerns separate and lets each screen decide what to render when offline. A wrapper component would couple that logic to a specific rendering strategy and make it less flexible if a screen needs different behavior in the future.
I agree with you. But I find the following approach slightly more ergonomic.
export const ItwIssuanceCredentialTrustIssuerScreen = (props: ScreenProps) => (
<RequiresConnectivity failureComponent={<OptionalCustomFailureComponent />}>
<ItwIssuanceCredentialTrustIssuer {...props} />
</RequiresConnectivity>
)It makes the intended behaviour explicit, keeps customization limited to the failure component and avoids requiring each screen to import or render the default failure component when no customization is needed.
RequiresConnectivity (or whatever name we are gonna use) clearly communicates "render this screen unless the offline condition applies.”
What do you think? Is there a specific use case that makes the hook approach preferable here?
Btw, a navigation middleware that replaces routes based on the connectivity state could probably solve 99% of these problems 😁
Co-authored-by: Damiano Plebani <hello@damianoplebani.com>
There was a problem hiding this comment.
As I said in another comment, why not a WithDebugEnabled component that we can use when we need something debug-gated?
3 participants
Short description
The goal of this PR is to complete the migration from HOCs and class components to React hooks and functional components, aligning the codebase with modern React best practices while preserving existing functionality.
This PR refactors several components and utilities to replace HOCs with hooks, converts key class components to functional components, and removes obsolete HOCs that are no longer needed. These changes simplify the codebase, improve readability and maintainability, and provide a more consistent development experience.
As part of completing this migration, the following ESLint rules have also been removed since they are no longer applicable:
List of changes proposed in this pull request
RootContainercomponent from a Redux-connected class component to a functional component using hooks for state and side effects, improving clarity and reducing boilerplatewithDebugEnabledHOC with auseDebugEnabledhook and updated all usages inDebugPrettyPrint,DebugView, and related files to use the hook. This allows for simpler and more idiomatic conditional rendering based on debug modewithAppRequiredUpdateHOC with auseAppRequiredUpdatehook, simplifying feature update checks and making the code more hook-centricCieCardReadingAnimationfrom a class component to a functional component using hooks, aligning with modern React practiceswithLoadingSpinnerHOC, reducing unnecessary abstractions and codeHow to test
Ensure the app works as it did before the migration from HOCs/class components to hooks.