PIN-7318: Added NotificationConfig page and its components #1352
Conversation
|
borgesis95
force-pushed
the
PIN-7318_configuration_notifications_page
branch
from
6df6c0c to
aba7bec
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
borgesis95
force-pushed
the
PIN-7318_configuration_notifications_page
branch
from
66554f9 to
126fc62
Compare
borgesis95
force-pushed
the
PIN-7318_configuration_notifications_page
branch
from
24bf3d0 to
07a6b9a
Compare
|
borgesis95
requested review from
Carminepo2 and
ruggerocastagnola
as code owners
sandrotaje
left a comment
sandrotaje
left a comment
There was a problem hiding this comment.
It looks good to me, If I'm not wrong this PR is missing the link to the configuration page but I think it will be included in another PR.
|
| | UserNotificationConfig['emailNotificationPreference'] | ||
| | UserNotificationConfig['inAppNotificationPreference'] | ||
| ) => { | ||
| const unnecessaryKeys = ['preferenceChoice'] |
There was a problem hiding this comment.
Is there any way to type this?
There was a problem hiding this comment.
!! Too many as for my taste, can you try to remove it and make the typing work without them?
| const isEnabledShowPreferencesSwitch = (): boolean => { | ||
| return match(type) | ||
| .with('email', () => { | ||
| return preferenceChoice === 'ENABLED' | ||
| }) | ||
| .with('inApp', () => { | ||
| return !!preferenceChoice | ||
| }) | ||
| .exhaustive() | ||
| } |
There was a problem hiding this comment.
MINOR: any reason why this is a function and not directly a computer property?
| return () => { | ||
| subscription.unsubscribe() | ||
| } | ||
| }, [formMethods, formMethods.watch, formMethods.formState.isDirty, debouncedUpdate]) |
There was a problem hiding this comment.
The formMethods.formState.isDirty dependency in this useEffect makes me a bit worried.
What's the reason it is there? It's important for a specific behaviour?
Can you remove it? If it is important I would at least document it in a comment
| type: NotificationConfigType | ||
| } | ||
|
|
||
| export const NotificationConfigUserTab: React.FC<NotificationConfigUserTabProps> = ({ |
There was a problem hiding this comment.
This component does TOO much.
I suggest at least to move all this form logic in a useNotificationConfigForm
| const SectionIcon = notificationSchema[sectionName].icon | ||
|
|
||
| return ( | ||
| <Box key={sectionName} data-testid={`config-section-${sectionName}`} sx={{ mb: 3 }}> |
There was a problem hiding this comment.
Can you move out this component?
| const notificationSchema: NotificationConfigSchema = match(type) | ||
| .with('inApp', () => { | ||
| return notificationConfigSchema | ||
| }) | ||
| .with('email', () => { | ||
| return notificationConfigSchema | ||
| }) | ||
| .exhaustive() |
There was a problem hiding this comment.
Is this for a future implementation when the inApp and email have their own scema?
| import { ConsumerIcon, ProviderIcon, MyTenantIcon } from '@/icons' | ||
| import CodeIcon from '@mui/icons-material/Code' | ||
|
|
||
| export function useNotificationConfigHook(type: NotificationConfigType) { |
There was a problem hiding this comment.
The Hook suffix communicates nothing, can you find a better one? What about useGetNotificationConfigSchema?
| const { isAdmin } = AuthHooks.useJwt() | ||
|
|
||
| const { data: user } = TenantHooks.useGetActiveUserParty() | ||
| const { data: tenantEmailNotifictionConfigs } = useQuery({ |
There was a problem hiding this comment.
typo
| const { data: tenantEmailNotifictionConfigs } = useQuery({ | |
| const { data: tenantEmailNotificationConfigs } = useQuery({ |
| const { data: tenantEmailNotifictionConfigs } = useQuery({ | ||
| ...NotificationQueries.getTenantNotificationConfigs(), | ||
| }) |
There was a problem hiding this comment.
No need to spread when you have nothing to add, it's a waste of performance
| const { data: tenantEmailNotifictionConfigs } = useQuery({ | |
| ...NotificationQueries.getTenantNotificationConfigs(), | |
| }) | |
| const { data: tenantEmailNotifictionConfigs } = useQuery(NotificationQueries.getTenantNotificationConfigs()) |
4 participants
No description provided.