Skip to content
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

Merged
merged 28 commits into from
May 2, 2024

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Apr 29, 2024

No description provided.

Copy link
Member

@fzaninotto fzaninotto left a 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!

packages/ra-ui-materialui/src/field/ImageField.tsx Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/field/TranslatableFields.tsx Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/field/TranslatableFields.tsx Outdated Show resolved Hide resolved
export type PreferencesEditorContextValue = {
editor: ReactElement | null;
setEditor: React.Dispatch<React.SetStateAction<ReactElement | null>>;
preferenceKey: string | null;
Copy link
Member

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?

Copy link
Collaborator Author

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);
Copy link
Member

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) ?? [];
Copy link
Member

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;
Copy link
Member

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

@djhi djhi changed the title Fixl remaining strictNullCheck errors in ra-ui-materialui Fix remaining strictNullCheck errors in ra-ui-materialui Apr 30, 2024
@djhi djhi changed the title Fix remaining strictNullCheck errors in ra-ui-materialui [TypeScript] Fix remaining strictNullCheck errors in ra-ui-materialui Apr 30, 2024
Co-authored-by: Francois Zaninotto <francois@marmelab.com>
Co-authored-by: Francois Zaninotto <francois@marmelab.com>
Co-authored-by: adrien guernier <adrien@marmelab.com>
@@ -41,8 +40,15 @@ export const CreateView = (props: CreateViewProps) => {
);
};

export type CreateViewProps = CreateProps;

export interface CreateViewProps {
Copy link
Member

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.

@fzaninotto fzaninotto merged commit ab27b44 into next May 2, 2024
12 checks passed
@fzaninotto fzaninotto deleted the fix-strict-null-checks-mui branch May 2, 2024 11:29
@fzaninotto
Copy link
Member

\o/

@fzaninotto fzaninotto mentioned this pull request May 2, 2024
24 tasks
@fzaninotto fzaninotto added this to the 5.0.0 milestone May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review v5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants