-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[TypeScript] Fix remaining strictNullCheck errors in ra-ui-materialui #9797
Conversation
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're getting close!
export type PreferencesEditorContextValue = { | ||
editor: ReactElement | null; | ||
setEditor: React.Dispatch<React.SetStateAction<ReactElement | null>>; | ||
preferenceKey: string | null; |
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 wonder why you prefer null to undefined here? Is this change necessary?
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 could but it looked more coherent with the other props
@@ -8,7 +8,7 @@ import { | |||
export const PreferencesEditorContextProvider = ({ children }) => { | |||
const [isEnabled, setIsEnabled] = useState(false); | |||
const [editor, setEditor] = useState<ReactElement | null>(null); | |||
const [preferenceKey, setPreferenceKey] = useState<string>(); | |||
const [preferenceKey, setPreferenceKey] = useState<string | null>(null); |
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.
you could default to undefined
), | ||
} | ||
: null | ||
)?.filter(column => column != null) ?? []; |
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 you should use an emptyArray const defined outside of the element to avoid rerenderings when the effect returns a different empty array object (modified 3 times)
@@ -73,6 +73,7 @@ export const Configurable = (props: ConfigurableProps) => { | |||
} | |||
|
|||
const handleOpenEditor = () => { | |||
if (!setEditor) return; |
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 we should rather throw an error in this case
Co-authored-by: Francois Zaninotto <[email protected]>
Co-authored-by: Francois Zaninotto <[email protected]>
Co-authored-by: adrien guernier <[email protected]>
@@ -41,8 +40,15 @@ export const CreateView = (props: CreateViewProps) => { | |||
); | |||
}; | |||
|
|||
export type CreateViewProps = CreateProps; | |||
|
|||
export interface CreateViewProps { |
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 it should extend the base div props (e.g. style
), otherwise valid HTML attributes, that are passed down to the root components, con't be accepted. This also allows you to get rid of className
. Same for the other 2.
\o/ |
No description provided.