Feat filters persistence #1437
Feat filters persistence #1437
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1437 +/- ##
==========================================
- Coverage 12.03% 11.97% -0.07%
==========================================
Files 272 273 +1
Lines 6969 7013 +44
Branches 1311 1314 +3
==========================================
+ Hits 839 840 +1
- Misses 4965 5005 +40
- Partials 1165 1168 +3
Continue to review full report at Codecov.
|
Conflicts: platform/core/src/utils/index.js platform/core/src/utils/index.test.js
|
Test summary
Run details
View run in Cypress Dashboard ➡️ Failures
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
||||||||||||||||||||||||||
|
We didn't run Visual Regression Tests after @galelis's Hotkeys update, so this test likely just needs tweaked. The Percy flavored Cypress tests aren't really trying to "catch" anything, they just use Cypress to setup the app for reproducible screen shot states.
That error and the file that it's originating should be enough to resolve it, as it should just be a string comparison/check. |
|
|
||
| const memoizedList = useMemo(() => { | ||
| return ( | ||
| <StudyList |
There was a problem hiding this comment.
Don't component only re-render if one of their props has updated? Does useMemo save us some CPU here? (why not wrap all component in useMemo?)
| fetchStudies(newStudyListSessionProps); | ||
| }, | ||
| [studyListSessionProps] | ||
| ); |
There was a problem hiding this comment.
Some repeat code here for setting these. Would it make more sense to shift from individual pieces of state to a "filter + sort" object, and then handle all of the callback/session/updates in once place?
There was a problem hiding this comment.
yeah we could, but to avoid having component re-render multiple times and considering things might change simultaneously (i.e page+filter, sort+something else...) this implementation ensure unicity and scalability. We can loop through it for any change if you desire to
| displaySize, | ||
| server, | ||
| ] | ||
| [server, displaySize] |
There was a problem hiding this comment.
Does useCallback net us much of a benefit here? Is the readability loss worth the performance gain?
There was a problem hiding this comment.
useCallback brings us mainly two things:
- performance
- unicity
From my understanding, considering React`s new way of coding, this method is not less readable, is it? We can change, add comments :D, your call my friend
| setSearchStatus({ error: null, isSearchingForStudies: false }); | ||
| }, FETCH_DEBOUNCE_TIME), | ||
| [server, displaySize] | ||
| ); |
There was a problem hiding this comment.
We could leverage our useDebounce on a more complex filters + sort object. When the debounce elapses, we could fetchData and store/cache the filter + sort in session storage?
There was a problem hiding this comment.
yes we could by to preserve the current user experience flow its kept as its.
To do such change the effort will be higher.
Currently the flow is: child event (ex: key type on filter field), tell parent, do some stuff on parent, update child. I.e parent is totally responsible to process things and update child. With such implementation the updating of child is preserved, session always has the real data, but fetch is called only sometimes..
Totally makes sense to be the way you mentioned but, there are two problems I see:
- Integration between different child. I.e. pieces need to be in sync: When fetching list and page must be updated, ...
- It will cause some refactoring on child end.
| ...currentProps, | ||
| ...newProps, | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The "new props" don't really conflict with currentProps. There's no cache overlap?
There was a problem hiding this comment.
I guess I ddint get your point here.
| let inputType = fieldInputType; | ||
| if (clearable && inputType === 'text') { | ||
| inputType = 'search'; | ||
| } |
There was a problem hiding this comment.
I'm not sure I follow what's going on here.
There was a problem hiding this comment.
At user experience: add x icon to remove text on a input field
At dev level: use inputType as search when field is clearable
ladeirarodolfo
force-pushed
the
feat-filters-persistence
branch
from
c0285b7
to
cf7c19d
Compare
|
base has changed, read more here #3477 |
|
This is an unfinished but valuable work, we should include it in v3 |
|
This PR is being closed in favour of this new PR #3749 which targets the most current master branch. Thanks @ladeirarodolfo for the inspiration, ideas, code and unit tests. |
sedghi
added
fixed-in-dev-await-release
PR Checklist
@mentiona maintainer to request a reviewIt includes
Screenshots