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

Introduce RecordContext and Base components for Edit, Create and Show #5422

Merged
merged 22 commits into from
Oct 27, 2020

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Oct 19, 2020

No description provided.

@djhi djhi added the WIP Work In Progress label Oct 19, 2020
@djhi djhi requested a review from fzaninotto October 19, 2020 14:45
@djhi djhi changed the title Introduce EditContext, EditBase and RecordContext Introduce RecordContext and Base components for Edit, Create and Show Oct 20, 2020
@djhi djhi added RFR Ready For Review WIP Work In Progress and removed WIP Work In Progress RFR Ready For Review labels Oct 20, 2020
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.

Awesome! This will ease Edit, Create and Show views customization big time.

packages/ra-core/src/controller/RecordContext.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/controller/RecordContext.tsx Outdated Show resolved Hide resolved
packages/ra-core/src/controller/SaveContext.tsx Outdated Show resolved Hide resolved
title,
version,
...rest
} = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CreateView should use the CreateContext to get the controller props. See how I did in ListView

packages/ra-ui-materialui/src/detail/EditActions.tsx Outdated Show resolved Hide resolved
undoable,
version,
...rest
} = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use useEditContext to get controller props

packages/ra-ui-materialui/src/detail/ShowActions.tsx Outdated Show resolved Hide resolved
title,
version,
...rest
} = props;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grab the show controller props from useShowContext

@djhi djhi added RFR Ready For Review WIP Work In Progress and removed WIP Work In Progress RFR Ready For Review labels Oct 22, 2020
Copy link
Contributor

@Luwangel Luwangel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So many changes. Well done!

packages/ra-core/src/controller/details/CreateBase.tsx Outdated Show resolved Hide resolved
packages/ra-ui-materialui/src/button/SaveButton.tsx Outdated Show resolved Hide resolved
@djhi djhi requested a review from fzaninotto October 22, 2020 13:42
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.

Almost there!

import { Record } from '../types';

/**
* Context to store the result of the useRecord() hook.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Context to store the result of the useRecord() hook.
* Context to store the current record.

/**
* Context to store the result of the useRecord() hook.
*
* Use the useRecordContext() hook to read the context. That's what the Edit and Show components do in react-admn.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Use the useRecordContext() hook to read the context. That's what the Edit and Show components do in react-admn.
* Use the useRecordContext() hook to read the context. That's what the Edit and Show components do in react-admin.

* import { useEditController, EditContext } from 'ra-core';
*
* const Edit = props => {
* const controllerProps = useEditController(props);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bad code example. Shouldn't it be:

const { record }= useEditController(props);
return (
     <RecordContextProvider value={record}>
        ...
    </RecordContextProvider>
);

??

};

/**
* Hook to read the record from a context which provide one, such as the EditContext or ShowContext.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Hook to read the record from a context which provide one, such as the EditContext or ShowContext.
* Hook to read the record from a RecordContext.

>(
context: RecordType
) => {
return pick(context, ['record']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking: every time something changes in a context (e.g. the selected ids in the list), the pick command will return a new instance of the record, re-rendering everything.

I think it's not a premature optimization to memoize the picking.

*/
export const useRecordContext = <
RecordType extends Record | Omit<Record, 'id'> = Record
>() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you must expect a record to be passed here to allow backwards compatibility

* saving
* } = useSaveContext();
*/
export const useSaveContext = () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must accept props as params for BC

export const useSaveContext = () => {
const context = useContext(SaveContext);

if (!context) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will never happen: the default context is an object

examples/simple/src/comments/CommentEdit.js Outdated Show resolved Hide resolved
examples/simple/src/comments/CommentEdit.js Outdated Show resolved Hide resolved
packages/ra-core/src/controller/RecordContext.tsx Outdated Show resolved Hide resolved
@fzaninotto fzaninotto merged commit 22d5510 into next Oct 27, 2020
@fzaninotto fzaninotto deleted the edit-base branch October 27, 2020 16:46
@fzaninotto
Copy link
Member

I'm so happy to have this merged!

@fzaninotto fzaninotto added this to the 3.10.0 milestone Oct 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants