Use React data-view for Kafka clusters table#2539
Conversation
MikeEdgar
force-pushed
the
data-view-research
branch
6 times, most recently
from
817819b to
cc7e9d4
Compare
MikeEdgar
force-pushed
the
data-view-research
branch
3 times, most recently
from
c66f136 to
5f2c265
Compare
MikeEdgar
changed the title
MikeEdgar
force-pushed
the
data-view-research
branch
2 times, most recently
from
8dee1e9 to
536444a
Compare
MikeEdgar
force-pushed
the
data-view-research
branch
from
536444a to
50489c3
Compare
MikeEdgar
force-pushed
the
data-view-research
branch
from
50489c3 to
d09b70b
Compare
alexcreasy
left a comment
alexcreasy
left a comment
There was a problem hiding this comment.
Overall it's a really nice improvement, there's a couple of points I've raised - the one about ignoring the api error response is only potentially serious issue. The rest are just generally some small changes to promote best practice.
| * synthetic URLSearchParams that includes the current page number. | ||
| * Uses local state to avoid flashing during navigation. | ||
| */ | ||
| const paginationSearchParams = useMemo(() => { |
There was a problem hiding this comment.
Just wondering why you have 2 memos that manipulate URL params and whether it might make sense condense them into 1 to avoid any potential strange issues or race conditions? (
There was a problem hiding this comment.
I was seeing some race conditions using a single memo to manipulate the params for both useDataViewSort and useDataViewPagination. I couldn't really narrow down to why or where it was happening, but I am definitely open to doing this with less code/functions if we can.
| }, [onSetFilters]); | ||
|
|
||
| // Custom hook for text filter handlers | ||
| const useTextFilterHandlers = (filterId: string) => { |
There was a problem hiding this comment.
This needs to be defined elsewhere, it's a strong convention in React that hooks need to be declared at the top level. You could move it outside the component declaration in this file, or move it to its own file.
There was a problem hiding this comment.
I moved this hook at the top level outside of the component. Is it mainly just a convention or are there technical reasons for doing so? I'm all for sticking to conventions, just curious.
Signed-off-by: Michael Edgar <medgar@redhat.com>
Signed-off-by: Michael Edgar <medgar@redhat.com>
MikeEdgar
force-pushed
the
data-view-research
branch
from
d09b70b to
3be1148
Compare
|
@alexcreasy I believe I've addressed your review comments, but let me know if something is still missing or out of place. The PR branch has been rebased, but the original commit is unchanged and your feedback is addressed in the second commit. Thanks! |
|
3 participants
For #2501
Subsequent changes will be coming to migrate all tables to use the new component.