SCC-4193 - Optimize state in bib page #341
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
dgcohen
changed the title
CHANGELOG
Outdated
| @@ -22,6 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Updated | |||
|
|
|||
| - Bib page state optimizations (SCC-4193) | |||
There was a problem hiding this comment.
Update this once the merge conflicts have been fixed.
| const viewAllLoadingMessage = | ||
| bib.getItemsViewAllLoadingMessage(filtersAreApplied) | ||
| const viewAllLoadingMessage = `Loading all ${ | ||
| filtersAreApplied ? "matching" : numItemsTotal | ||
| } items. This may take a few moments...` |
There was a problem hiding this comment.
getItemsViewAllLoadingMessage is only used here, why not change it instead of moving the functionality here? Or at least delete the declaration from the Bib model.
There was a problem hiding this comment.
Good catch, i removed it in the Bib model since it's only used here, but can add it back if you'd like greater separation of logic from presentation.
There was a problem hiding this comment.
Either approach is ok but I brought it up just as a question.
…/optimize-state-in-bib-page
| const viewAllLoadingMessage = | ||
| bib.getItemsViewAllLoadingMessage(filtersAreApplied) | ||
| const viewAllLoadingMessage = `Loading all ${ | ||
| filtersAreApplied ? "matching" : numItemsTotal | ||
| } items. This may take a few moments...` |
There was a problem hiding this comment.
Either approach is ok but I brought it up just as a question.
Nothing noticeable but Edwin suggested this change because we don't need to refresh the whole bib in state. @EdwinGuzman would it be worth performing benchmark tests to see if this is necessary? |
|
@dgcohen yes, that'd be great. How about we keep this on hold for a bit and you run some perf tests (lighthouse or another tool) with the current site against this PR? The difference may be negligible but code-wise, I think this is a helpful update. |
Ticket:
This PR does the following:
How has this been tested?
NA
Accessibility concerns or updates
NA
Checklist: