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

Merge @comet/blocks-admin into @comet/cms-admin #3049

Merged
merged 24 commits into from
Feb 11, 2025

Conversation

johnnyomair
Copy link
Collaborator

@johnnyomair johnnyomair commented Jan 8, 2025

Description

The dedicated @comet/blocks-admin package was originally introduced to support projects without CMS parts.
It turned out that this is never the case, so the separation doesn't make sense anymore.
Therefore, the @comet/blocks-admin is merged into @comet/cms-admin.

Acceptance criteria

Open TODOs/questions

Further information

@johnnyomair johnnyomair self-assigned this Jan 8, 2025
@johnnyomair johnnyomair force-pushed the merge-blocks-admin-into-cms-admin branch from 3b8644f to c842237 Compare January 14, 2025 08:40
@johnnyomair johnnyomair force-pushed the merge-blocks-admin-into-cms-admin branch from c842237 to 89c0e1a Compare January 14, 2025 09:33
@johnnyomair
Copy link
Collaborator Author

I've reviewed the newly added exports to @comet/cms-admin and performed the following changes:

Multiple exports that shouldn't be used have been removed from the public API
- CannotPasteBlockDialog
- ClipboardContent
- useBlockClipboard
- Collapsible
- CollapsibleSwitchButtonHeader
- usePromise

Multiple exports that were too generic have been renamed
- createCompositeSetting -> createCompositeBlockField
- createCompositeSettings -> createCompositeBlockFields
- IPreviewContext -> BlockPreviewContext
- PreviewStateInterface -> BlockPreviewStateInterface

There are some exports where I'm not sure what to do: DispatchSetStateAction, SetStateAction, SetStateFn, and resolveNewState. These are all related to the blocks state, which is a React state most of the time. Dispatch and SetStateAction are even duplicates of / similar to types defined by React. What do you think?

Also, are there any other changes you deem necessary?

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
4.4% Duplication on New Code (required ≤ 3%)
D Security Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@johnnyomair johnnyomair marked this pull request as ready for review January 14, 2025 09:49
@fraxachun fraxachun removed their request for review January 14, 2025 09:59
thomasdax98
thomasdax98 previously approved these changes Jan 28, 2025
@johnnyomair
Copy link
Collaborator Author

There are some exports where I'm not sure what to do: DispatchSetStateAction, SetStateAction, SetStateFn, and resolveNewState. These are all related to the blocks state, which is a React state most of the time. Dispatch and SetStateAction are even duplicates of / similar to types defined by React. What do you think?

@thomasdax98 what do you think about this?

@nsams
Copy link
Member

nsams commented Jan 28, 2025

There are some exports where I'm not sure what to do: DispatchSetStateAction, SetStateAction, SetStateFn, and resolveNewState. These are all related to the blocks state, which is a React state most of the time. Dispatch and SetStateAction are even duplicates of / similar to types defined by React. What do you think?

are these really used in applications? best would be if we could drop them....

@johnnyomair
Copy link
Collaborator Author

johnnyomair commented Feb 3, 2025

are these really used in applications? best would be if we could drop them....

SetStateAction and SetStateFn appear to be unused, DispatchSetStateAction can be replaced with Dispatch<SetStateAction<T>> from react instead. resolveNewState, however, is used 73 times in our projects and can't be easily replaced. Basically it's a helper to transform a React setState call into the next state by checking if the updater is a function or an object. I'd say we should keep it. Maybe we can rename it and move it to @comet/admin, but this is out of scope of this PR.

@johnnyomair johnnyomair force-pushed the merge-blocks-admin-into-cms-admin branch from 06f2162 to 52fafb5 Compare February 3, 2025 14:07
@johnnyomair johnnyomair requested a review from nsams February 4, 2025 16:21
@johnnyomair johnnyomair merged commit 9ddb6c4 into next Feb 11, 2025
12 checks passed
@johnnyomair johnnyomair deleted the merge-blocks-admin-into-cms-admin branch February 11, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants