STCOR-952 break QueryStateUpdates out to their own class-based component. #1604
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
zburke
reviewed
Feb 26, 2025
zburke
approved these changes
Mar 3, 2025
|
zburke
pushed a commit
that referenced
this pull request
Mar 12, 2025
…ent. (#1604) Changes from [App reordering](https://folio-org.atlassian.net/browse/STCOR-936) - specifically a functional refactor of `MainNav` unearthed a handful of errors at the app level, causing behaviors with logic that updates query parameters/location. This PR takes the previous code from the class-based `MainNav` and places it in its own component: `QueryStateUpdater` - naming aside, this removes the potentially offending logic from `MainNav`, leaving it to the task of simply rendering the navigation/menus of the UI. The useEffect implementations of the functional `MainNav` were written to mirror the `componentDidMount` and `componentDidUpdate` lifecycle methods of the class-based version. When the `location` was added to the `cDM` dependency array in [This PR](#1600), this meant that the logic would no longer execute stricly on mount/dismount - but anytime that the `location` updated... this would cause page refreshes, as the logic would find a black `prevQueryState` every time and always update the location, causing a refresh. `componentDidUpdate` carried around its `prevProps`, unlike functional react, where a state might be needed to accomplish such a comparison - but this doesn't seem like an ideal solution for the functional version, so here we are. This is better because separately concerned logic is separated... worse because the separated logic still has all of its tentacles into the App layer and seems like a great example of a component that does too much - why wouldn't/couldn't updating queryState/location actually just live at the application level somehow? A question for future FOLIO... (cherry picked from commit 9cbaa1b)
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.
Changes from App reordering - specifically a functional refactor of
MainNavunearthed a handful of errors at the app level, causing behaviors with logic that updates query parameters/location. This PR takes the previous code from the class-basedMainNavand places it in its own component:QueryStateUpdater- naming aside, this removes the potentially offending logic fromMainNav, leaving it to the task of simply rendering the navigation/menus of the UI.The useEffect implementations of the functional
MainNavwere written to mirror thecomponentDidMountandcomponentDidUpdatelifecycle methods of the class-based version. When thelocationwas added to thecDMdependency array in This PR, this meant that the logic would no longer execute stricly on mount/dismount - but anytime that thelocationupdated... this would cause page refreshes, as the logic would find a blackprevQueryStateevery time and always update the location, causing a refresh.componentDidUpdatecarried around itsprevProps, unlike functional react, where a state might be needed to accomplish such a comparison - but this doesn't seem like an ideal solution for the functional version, so here we are.This is better because separately concerned logic is separated... worse because the separated logic still has all of its tentacles into the App layer and seems like a great example of a component that does too much - why wouldn't/couldn't updating queryState/location actually just live at the application level somehow? A question for future FOLIO...