Refactor users table and pagination #381
Conversation
| <option value='both'>both</option> | ||
| <option value='true'>true</option> | ||
| <option value='false'>false</option> |
There was a problem hiding this comment.
How come this dropdown doesn't use the SVG, while the other one does?
| @@ -113,8 +103,8 @@ const UsersTable = () => { | |||
| className='absolute top-0 left-0 z-20 h-full w-full bg-white opacity-0' | |||
| > | |||
| <option value=''>Select filters</option> | |||
| {['past_due', 'canceled', 'active', 'deleted', null].map((status) => { | |||
| if (!statusOptions.includes(status as SubscriptionStatus)) { | |||
| {['past_due', 'cancel_at_period_end', 'active', 'deleted', null].map((status) => { | |||
There was a problem hiding this comment.
I think this was a mistake, the filtered returned nothing.
I'm guessing canceled is an old status.
| const value = parseInt(e.currentTarget.value); | ||
| if (data?.totalPages && value <= data?.totalPages && value > 0) { | ||
| setCurrentPage(value); | ||
| } |
There was a problem hiding this comment.
The old code had a bug and would break if you manually deleted the page number.
There was a problem hiding this comment.
I'm guessing you mean "manually edited"?
| @@ -88,41 +95,18 @@ export const getPaginatedUsers: GetPaginatedUsers<GetPaginatedUsersInput, GetPag | |||
| paymentProcessorUserId: true, | |||
| }, | |||
| orderBy: { | |||
| id: 'desc', | |||
| username: 'asc', | |||
There was a problem hiding this comment.
Sorting seems less random because users are displayed in the table.
| skipPages: number; | ||
| filter: { | ||
| emailContains?: string; | ||
| isAdmin?: boolean; | ||
| subscriptionStatusIn?: SubscriptionStatus[]; | ||
| }; |
There was a problem hiding this comment.
Separating the data (filter params) from the metadata (paging params).
| const [page, setPage] = useState(1); | ||
| const [email, setEmail] = useState<string | undefined>(undefined); | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [emailFilter, setEmailFilter] = useState<string>(''); |
There was a problem hiding this comment.
If we have undefined for isAdmin as empty state, I'd expect it to have for the emailFilter as well.
Below you are counting on the behavior when emailFilter is fasly anyways (emailFilter &&) - you could go all the way with explicit undefined.
There was a problem hiding this comment.
Okay, I guess it's because of setEmailFilter(e.currentTarget.value);, okay this looks like it's more justified. But in the name of consistency, maybe you could go for:
if (e.currentTarget.value === '') {
setEmailFilter(undefined);
} else {
setEmailFilter(e.currentTarget.value);
}
| filter: { | ||
| ...(emailFilter && { emailContains: emailFilter }), | ||
| ...(isAdminFilter !== undefined && { isAdmin: isAdminFilter }), | ||
| ...(subscriptionStatusFilter?.length > 0 && { subscriptionStatusIn: subscriptionStatusFilter }), |
There was a problem hiding this comment.
Can subscriptionStatusFilter ever be undefined or null?
| useEffect( | ||
| function backToPageOne() { | ||
| setCurrentPage(1); | ||
| }, | ||
| [emailFilter, subscriptionStatusFilter, isAdminFilter] | ||
| ); |
There was a problem hiding this comment.
For some reason, this, to me, feels like an odd way of writing it (I'm not saying it's wrong), I'd write something like this:
function backToPageOne() {
setCurrentPage(1);
},
useEffect(() => {
backToPageOne();
}, [emailFilter, subscriptionStatusFilter, isAdminFilter]);There was a problem hiding this comment.
I only learned about this trick recently. I like it because it allows you to name your inline useEffect function without forcing you to jump around with your eyes - you still read it sequentially, but can now tell what it's supposed to do.
Also, the anonymous useEffect arrow function that wraps backToPageOne can easily grow and include more logic, and then we're back at square one - an anonymous inline function
The only thing I don't like is the formatting. This is how I'd like it to look:
useEffect(function backToPageOne() {
setCurrentPage(1);
}, [emailFilter, subscriptionStatusFilter, isAdminFilter]);But prettier doesn't allow it.
Regardless of the formatting, I still like this trick above reasons. That said, if you think it's weird enough to throw people off, I'll change it.
Btw, this isn't supposed to be a useEffect anyway - we should be calling backToPageOne on each filter change, but I didn't have time to refactor that.
| @@ -56,8 +59,8 @@ const UsersTable = () => { | |||
| </label> | |||
| <div className='flex-grow relative z-20 rounded border border-stroke pr-8 outline-none bg-white transition focus:border-primary active:border-primary dark:border-form-strokedark dark:bg-form-input'> | |||
| <div className='flex items-center'> | |||
| {!!statusOptions && statusOptions.length > 0 ? ( | |||
| statusOptions.map((opt) => ( | |||
| {!!subscriptionStatusFilter && subscriptionStatusFilter.length > 0 ? ( | |||
There was a problem hiding this comment.
I'd drop the !!subscriptionStatusFilter here since it can't be undefined or null.
There was a problem hiding this comment.
Yeah, this is old, the change is included because of formatting.
I'll fix it up.
No description provided.