Move ResultTablesHeader to its own component#2666
Conversation
| getResultSets(props.rawResultSets, props.interpretation), | ||
| ); | ||
|
|
||
| return { |
There was a problem hiding this comment.
This is the only bit where it isn't perfectly replicated on the new component and I'm unsure if it's needed or not.
This is checking if the result set exists and if not setting the selected table and page to default values. I'm not sure how to trigger this state. I also think it should be covered by the useEffect hook in the header component which always sets the selected page to the value from the parsed result set.
| const [selectedPage, setSelectedPage] = React.useState( | ||
| `${parsedResultSets.pageNumber + 1}`, | ||
| ); | ||
| useEffect(() => { |
There was a problem hiding this comment.
Interestingly this effect hook might not strictly be needed. My brain says it should be needed to update selectedPage, but when I try removing it everything still works. 🤔
The case I'd expect to be different is when moving between query results. If I change from viewing the results of one query to a different query, then parsedResultSets is updated and we want selectedPage to also be reset.
Possibly it's because ResultTables is constructing a new copy of this component on every render, so we're always getting the initial calculated value. This could change as we continue to tweak ResultTables.
There was a problem hiding this comment.
I think it's worth keeping it - it does seem like the right thing to do.
|
|
||
| const onKeyDownHandler = useCallback( | ||
| (e: React.KeyboardEvent<HTMLInputElement>) => { | ||
| changePage(e.currentTarget.value); |
There was a problem hiding this comment.
Oops, I forgot the logic about checking if it's keycode 13. I'll add that back in.
The callback used to look like:
onKeyDown={(e) => {
if (e.keyCode === 13) {
choosePage((e.target as HTMLInputElement).value);
}
}}
but I accidentally removed the if condition.
There was a problem hiding this comment.
I've actually done e.key === "Enter" because e.keyCode is deprecated now. See https://www.w3.org/TR/uievents-key/#named-key-attribute-values for a list of possible values.
|
A note to any reviewers: I'm not hugely confident that this code is any good 😄 . I'm also not all that attached to it, so if it turns out to be too hard to review or you're also unsure if this PR is an improvement on the current state then I'm happy to just close it. This work is not essential and we can always try it again another time. |
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Sorry it took a while to review - the diff was a bit scary 😬
I'm not super confident in my review, but I think each code change I've seen makes sense and it's a positive step. I can't see anything wrong. 🚢
I'm sorry it was a difficult diff! Next time I do one of these I'll try harder to do each change separately. In particular I should have moved the code to the new file in a separate commit to transforming it. Thank you for persevering. ❤️ |
2 participants
I think we can reduce the size of the
ResultTablescomponent by pulling out to the bits for the header to a separate component. This header contains the pagination controls as well as the query name and file link.I've been testing it and as far as I can tell it's still working just as it was before.
Checklist
ready-for-doc-reviewlabel there.