-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor #99
Open
Raxxius
wants to merge
28
commits into
rcpch:live
Choose a base branch
from
Raxxius:refactor
base: live
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Refactor #99
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Refactoring
Process
Deep code dive (4 days)
Refactoring (4 days)
Changelist for Refactoring
Layering
Existing client has essentially empty layers in the App.js and page.js that are simply calling another layer. Removing those redundant layers reduced the depth of the project. Additionally the MeasurementSegment.js is a 556 line file with data management, display management, multiple useEffects and Semantic UI all within the same layer.
App.js is now the core data management layer, all useEffects and useStates are called and stored in this layer.
The page folder has been removed in its entirety, as it’s not relevant as the react app is being used as a component and not a page in itself.
MeasurementSegment has become the display layer SemantiGrid, containing Semantic UI logic and the display logic.
At this stage, no further addressing of deeper layers has been conducted.
Exported functions and hooks
Functions that are standalone have been removed from the App.js main into standalone file in functions folders. Similarly useEffects have been turned into custom hooks and moved to the hooks folder.
4 Custom hooks have been created, useErrorHandling, useSelectedTheme, useCheckForData useRcpchAPi have been created in the new hooks folder along with useGlobalState
Semantic UI constructor functions createSemanticPanes, createFormPanes and createTabPanes have been created in the components/SemanticGrid subcomponent folder
Global constants have been moved into assets/config.js
Separation of components
ErrorModal has been turned into a separate component
Semantic UI is now nearly entirely contained in the MeasurementSegment which acts as a display layer for
Conclusions
App.js is now a 252 line file with better separation of concerns. MeasurementSegment has become a display layer for Semantic Grid components MeasurementForm and ResultsForm.
Still to do
Review props and data layer position.
At this moment a very large number of props are being passed down to SemanticGrid, a proper assessment of what props can be moved from the App.js file to the SemanticGrid layer.
This will also allow for the complete removal of Semantic UI from the App.js layer.
Correct identification and grouping of props will assist in future updates.
MeasurementForm.js
This is another 600+ line single file that is used by the createFormPanes component, and it should be further separated into data management, subcomponents and functions.
Better commenting of files to allow for ease of understanding of components and their functions
Completing these steps will allow for easier completion of updates and maintenance to the client.