Remove objectWithout usage from elements package
#12209
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
swissspidy
added
Type: Code Quality
|
Plugin builds for 344e0a3 are ready 🛎️!
|
miina
reviewed
Sep 2, 2022
| @@ -25,18 +21,15 @@ import createNewElement from './createNewElement'; | |||
| import duplicateElement from './duplicateElement'; | |||
|
|
|||
| const duplicatePage = (oldPage) => { | |||
| // Remove title and postId for inserting the page. | |||
| // Remove title and templateId for inserting the page. | |||
There was a problem hiding this comment.
I tried to test this part and can't see when would a template / post ID be assigned at all. Thought that perhaps when inserting a page template and then duplicating it but doesn't seem like it. Were you able to test in which case we need to remove the title and ID when duplicating?
There was a problem hiding this comment.
Indeed, doesn't look like that happens at all.
I tried inserting default page templates and custom saved page templates, but there's no templateId leaking into this.
Looking at handlePageClick, it's specifically omitted:
So yeah, seems like it's safe to remove this comment & check.
For context, originally introduce in #7290 and then adjusted in #7318
|
Size Change: -102 B (0%) Total Size: 2.68 MB
ℹ️ View Unchanged
|
miina
approved these changes
Sep 5, 2022
spacedmonkey
approved these changes
Sep 5, 2022
barklund
approved these changes
Sep 5, 2022
| // There's an exception for the background shape that should not get all the default attributes. | ||
| if (attributes.isDefaultBackground) { | ||
| return objectWithout(newElement, ['backgroundColor']); | ||
| const { backgroundColor: _, ...newElementWithoutBgColor } = newElement; |
There was a problem hiding this comment.
Why rename it to _ if unused anyway?
There was a problem hiding this comment.
ESLint complained otherwise, surprisingly
timarney
approved these changes
Sep 6, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
I just noticed this is the only reason for the dependency on the
design-systempackage. That util is not used anywhere else (thestory-editorhas its own copy).Summary
Remove
objectWithoutusage fromelementspackage, removing thedesign-systempackage.Fixes a bug in
duplicatePagetoo, I think..?Also removes a similar usage from the
patternspackage that I stumbled upon, making the code a bit more readableRelevant Technical Choices
To-do
User-facing changes
Testing Instructions
This PR can be tested by following these steps:
Reviews
Does this PR have a security-related impact?
Does this PR change what data or activity we track or use?
Does this PR have a legal-related impact?
Checklist
Type: XYZlabel to the PRFixes #