-
Notifications
You must be signed in to change notification settings - Fork 1
feat: split domain table intom components with ui and logic with hocs #42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| /** | ||
| * Domain Table UI component. | ||
| * | ||
| * Renders domain table component but requires external state management via the `logic` prop. | ||
| * Use this when you need custom Domain Table or want to integrate with your own state. | ||
| * Alternatively, you can use hooks like `useDomainTableLogic` or `useDomainTableState` to build custom logic | ||
| * without starting from scratch. | ||
| * | ||
| * @example | ||
| * ```tsx | ||
| * const customLogic = { | ||
| * state: { showCreateModal: false, setShowCreateModal: (show: boolean) => void }, | ||
| * actions: { handleCreate: (type) => {...}, handleDelete: (id) => {...} }, | ||
| * domainTableActions: { createAction: async () => {...} } | ||
| * }; | ||
| * | ||
| * <DomainTableUI logic={customLogic} /> | ||
| * ``` | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we changed the approach we were following on JSdocs? I know that @rax7389 has been thinking on doing some changes, but just to confirm if we should add this example or not
| @@ -0,0 +1,35 @@ | |||
| import * as React from 'react'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with-core-client is no longer needed, it was replaced by with-services
|
|
||
| return <Component {...props} logic={domainTableLogic} />; | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into domain-table.tsx? Feel free to not use a HOC for this and just pass the props to children
| const { | ||
| domains, | ||
| providers, | ||
| isFetching, | ||
| isCreating, | ||
| isVerifying, | ||
| isDeleting, | ||
| isLoadingProviders, | ||
| fetchProviders, | ||
| fetchDomains, | ||
| onCreateDomain, | ||
| onVerifyDomain, | ||
| onDeleteDomain, | ||
| onAssociateToProvider, | ||
| onDeleteFromProvider, | ||
| } = useDomainTable({ | ||
| createAction, | ||
| verifyAction, | ||
| deleteAction, | ||
| associateToProviderAction, | ||
| deleteFromProviderAction, | ||
| customMessages, | ||
| }); | ||
|
|
||
| const { | ||
| showCreateModal, | ||
| showConfigureModal, | ||
| showVerifyModal, | ||
| showDeleteModal, | ||
| verifyError, | ||
| selectedDomain, | ||
| setShowCreateModal, | ||
| setShowConfigureModal, | ||
| setShowDeleteModal, | ||
| handleCreate, | ||
| handleVerify, | ||
| handleDelete, | ||
| handleToggleSwitch, | ||
| handleCloseVerifyModal, | ||
| handleCreateClick, | ||
| handleConfigureClick, | ||
| handleVerifyClick, | ||
| handleDeleteClick, | ||
| } = useDomainTableLogic({ | ||
| t, | ||
| onCreateDomain, | ||
| onVerifyDomain, | ||
| onDeleteDomain, | ||
| onAssociateToProvider, | ||
| onDeleteFromProvider, | ||
| fetchProviders, | ||
| fetchDomains, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were 2 different hooks, 1 for logic and 1 for api requests but you are removing one of them and mixing the code.
I think I would keep the 2 original hooks without changing anything, just moving both of them into the HOC and passing props to children
| export { useDomainTableLogic } from '@react/hooks/my-org/domain-management/use-domain-table-logic'; | ||
| export { useDomainTableState } from '@react/hooks/my-org/domain-management/use-domain-table-state'; | ||
| export * from '@react/hooks/my-org/domain-management/use-domain-table'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not use @react imports, for the moment only relative imports must be added
| } | ||
|
|
||
| return context; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use-config file already exists in /src/hooks/my-org-/config, we should not add it again
| import type { CustomOverrides, ThemeSettings } from './theme-types'; | ||
|
|
||
| /** | ||
| * Auth0 authentication details with optional React-specific properties. | ||
| */ | ||
| export type AuthDetails = Omit<AuthDetailsCore, 'accessToken'>; | ||
|
|
||
| /** | ||
| * Props for the Auth0ComponentProvider component. | ||
| */ | ||
| export interface Auth0ComponentProviderProps { | ||
| i18n?: I18nOptions; | ||
| themeSettings?: ThemeSettings; | ||
| authDetails: AuthDetails; | ||
| authDetails?: AuthDetails; | ||
| customOverrides?: CustomOverrides; | ||
| loader?: React.ReactNode; | ||
| } | ||
|
|
||
| /** | ||
| * Props for the InternalProvider component. | ||
| */ | ||
| export interface InternalProviderProps { | ||
| i18n?: I18nOptions; | ||
| authDetails?: AuthDetails; | ||
| } | ||
|
|
||
| /** | ||
| * Configuration for Auth0ComponentProvider excluding authentication details. | ||
| */ | ||
| export type Auth0ComponentConfig = Omit<Auth0ComponentProviderProps, 'authDetails' | 'i18n'>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you using updated code? I think none of this is needed
| /** | ||
| * BrandingTheme | ||
| * | ||
| * Controlled UL branding configuration. | ||
| */ | ||
| export type BrandingTheme = { | ||
| mode?: 'light' | 'dark' | 'system'; | ||
| primaryColor?: string; | ||
| borderRadius?: number; | ||
| fontFamily?: string; | ||
| [key: string]: unknown; | ||
| }; | ||
|
|
||
| /** | ||
| * CustomerOverrides | ||
| * | ||
| * Custom CSS variable overrides (e.g. "--button-radius": "6px"). | ||
| */ | ||
| export type CustomOverrides = Record<string, string>; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, this is old code.
I would start again with the latest code from main branch and just split domain-table component into:
domain-table-ui: presentational, receive 2 props (logicandapi)
Maybe we can find a better name for theapiprop 🤔 .domain-table: use theuseDomainTableanduseDomainTableLogichooks and pass data into the previous props todomain-table-uicomponent.
And then expose both components in domain-table so you can import them from other projects.
By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
We should update Domain Table component under IDP Management to separate API logic from the component itself.
We should create a high-order component that injects the logic, and export both components. Something like DomainTableComponent (with the logic) and DomainTableUIComponent (without the logic).
In addition, we will expose the hooks we use, just in case the users want to reuse them too.
We have separated all main blocks into 2 components:
We are exposing:
References
Testing
Checklist