Refactor Switchers and upding isAdmin
#378
Conversation
sodic
changed the title
isAdmin
There was a problem hiding this comment.
I moved this file into a more appropriate folder.
For both switchers, I extracted isOn and setIsOn as props.
SwitcherOne could have only been used for updating isAdmin. It's now general
| import { updateIsUserAdminById } from 'wasp/client/operations'; | ||
| import { type User } from 'wasp/entities'; | ||
|
|
||
| const AdminSwitch = ({ id, isAdmin }: Pick<User, 'id' | 'isAdmin'>) => { |
There was a problem hiding this comment.
For some reason I prefer the function AdminSwitch(...) {} way of defining components. One objective reason would be they are named functions then.
There was a problem hiding this comment.
I agree 100% (some more reasons here), but I wanted to stick with the repo's convention (most if not all components are arrow functions).
That said, I see I've messed it up in other places, so I'll probably make those arrow functions too if that's alright.
There was a problem hiding this comment.
Why not start at some point writing the named functions variant if we feel like it's better and make into a task to rewrite all the arrow functions? :)
There was a problem hiding this comment.
It's up for discussion definitely, but IMO consistency is more important than the specific choice in this case.
If we did that, the codebase would exist in in a non-consistent state for who a long time (possibly even forever).
However, I did a quick search, and it seems we're already pretty inconsistent 😄
OpenSaas (and Wasp) are a mix of arrows and normal functions.
So, with that excuse, I'll rewrite all my refactors to use function 👍
| @@ -109,7 +115,11 @@ const UsersTable = () => { | |||
| <option value=''>Select filters</option> | |||
| {['past_due', 'canceled', 'active', 'deleted', null].map((status) => { | |||
| if (!statusOptions.includes(status as SubscriptionStatus)) { | |||
| return <option value={status || ''}>{status ? status : 'has not subscribed'}</option>; | |||
| return ( | |||
| <option key={status} value={status || ''}> | |||
There was a problem hiding this comment.
Do we maybe want to use the more modern ?? operator here?
There was a problem hiding this comment.
I refactored this in #382
| @@ -201,7 +205,13 @@ const FormElements = ({ user }: { user: AuthUser }) => { | |||
| <option value=''>Canada</option> | |||
| </select> | |||
| <span className='absolute top-1/2 right-4 z-10 -translate-y-1/2'> | |||
| <svg width='24' height='24' viewBox='0 0 24 24' fill='none' xmlns='http://www.w3.org/2000/svg'> | |||
| <svg | |||
There was a problem hiding this comment.
Why not extract these SVGs as well?
There was a problem hiding this comment.
Forgot, will do, thanks!
| import { cn } from '../../../client/cn'; | ||
| import { ChangeEventHandler } from 'react'; | ||
|
|
||
| const SwitcherOne = ({ |
| onChange, | ||
| }: { | ||
| isOn: boolean; | ||
| onChange: ChangeEventHandler<HTMLInputElement>; |
There was a problem hiding this comment.
Usually, I've seen this defined as simply onChange: (value: boolean) => void and then in the input you use onChange={e => onChange(e.target.checked) }
There was a problem hiding this comment.
You're right, this is an abstraction leak. I'll change it up.
Description
Refactoring OpenSaas's user module
Contributor Checklist