-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FIX] - Pagination Component redundant fetches #115
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's crazy how much better of a user experience this is 🧑🍳
Couple of things:
- I think the flow would be even better if when you click enter, you're deselected from the box
- Not sure if this is out of the scope of this ticket, but do we also want to consider disabling the inputs while the records are being fetched? cc @Safewaan
// Only fetch records if a valid page num is present AND the page num has changed | ||
const fetchRecords = () => { | ||
if (!Number.isNaN(userPageNum) && userPageNum !== pageNum) { | ||
getRecords(userPageNum); | ||
} | ||
} | ||
|
||
const handleBlur = (event: React.FocusEvent<HTMLInputElement>) => { | ||
// Treat the enter key as an alt method of triggering onBlur (lose focus) | ||
const handleKeyUp = (event: any) => { | ||
if (event.keyCode === 13) { | ||
event.target.blur(); | ||
} | ||
} | ||
|
||
const handleBlur = () => { | ||
if (Number.isNaN(userPageNum)) { | ||
setUserPageNum(pageNum); | ||
return; | ||
} | ||
fetchRecords() | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This control flow is a little weird, so I thought I'd comment it to explain:
Two ways to trigger a fetch:
- Exiting focus from the textbox (Clicking outside of it) ; handleBlur
- Hitting the enter key ; handleKeyUp
handleBlur:
- Checks if number is present ; if not, reset to the previous number and terminate
- If a valid number is present, initiate a fetch
handleKeyUp:
- Checks if key-up (triggered once a user "stops" pressing a key) is == 13 (the key code for the
ENTER
key) ; if true, then this will force ablur
event (triggering thehandleBlur
function)
Let's see what the performance of pagination looks like after the improvements and discuss with the designers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM THIS IS 🔥 (fire)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is sickkkkk lets goo 👨🍳
GitHub Issue link
Pagination Component - Prevent redundant / unintentional fetches
Implementation description
fetchRecords
function to actually perform the getonKeyUp
event handler to trigger fetchhandleNumberInputChange
function to only update the input valueSteps to test
tab
or clicking outside of it) will refetch log recordsWhat should reviewers focus on?
Checklist