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

feat(app-headless-cms): enable full-screen editor for content entries #4443

Open
wants to merge 6 commits into
base: next
Choose a base branch
from

Conversation

leopuleo
Copy link
Contributor

Changes

This PR introduces a new experimental full-screen editor for headless CMS entries. The feature is under the feature flag: allowCmsFullScreenEditor: true.

As we work on this feature, we enable the EntryForm Header to be decorated. This capability is essential for its development.
CleanShot 2024-12-12 at 18 34 31@2x

How Has This Been Tested?

Manually

@webiny-bot webiny-bot added this to the 5.42.0 milestone Dec 12, 2024
@leopuleo leopuleo requested a review from Pavel910 December 12, 2024 17:42
@leopuleo leopuleo self-assigned this Dec 12, 2024
@leopuleo
Copy link
Contributor Author

/cypress

Copy link

Cypress E2E tests have been initiated (for more information, click here). ✨

Comment on lines +25 to +27
const isNewEntry = useMemo(() => !entry.meta?.title, [entry.meta]);
const version = useMemo(() => entry.meta?.version ?? null, [entry.meta]);
const status = useMemo(() => entry.meta?.status ?? null, [entry.meta]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

All three hooks will always run, if entry.meta changes. You might as well combine all 3 into 1 hook, which returns an object { isNewEntry, version, status }. Otherwise, use specific dependencies on each hook, like entry.meta?.title, entry.meta?.version, etc.

const isNewEntry = useMemo(() => !entry.meta?.title, [entry.meta]);
const version = useMemo(() => entry.meta?.version ?? null, [entry.meta]);
const status = useMemo(() => entry.meta?.status ?? null, [entry.meta]);
const modelName = useMemo(() => model.name, [model.name]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This useMemo is completely pointless. You'll spend more processing power to execute the hook, than gain anything. You're memoizing that same string value, which you have in the dependencies array :)

} from "./FullScreenContentEntry.styled";

export const FullScreenContentEntryHeaderLeft = () => {
const { model } = useModel();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This hook is redundant, because useContentEntry() hook below, has access to that same model, just add a contentModel key.

Comment on lines +21 to +24
const title = useMemo(
() => entry?.meta?.title || `New ${model.name}`,
[entry.meta, model.name]
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These type of memoizations are adding more calculations than they're optimizing. These simple assingments can be executed on each render, especially since the output is a primitive string.

});

export const FullScreenContentEntry = () => {
return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add feature flag here, and remove it from all other components:

if (!featureFlags.allowCmsFullScreenEditor) {
    return null;
}

If the feature is disabled, you simply don't decorate anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants