-
Notifications
You must be signed in to change notification settings - Fork 163
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
Wrap in List #5722
Wrap in List #5722
Conversation
#12613 Bundle Size — 62.18MiB (+0.01%).
Warning Bundle contains 52 duplicate packages – View duplicate packages Bundle metrics
Bundle size by type
Bundle analysis report Branch feat/wrap-in-list Project dashboard |
export interface MapInsertionPath { | ||
type: 'MAP_INSERTION' | ||
intendedParentPath: StaticElementPath | ||
insertBehavior: ConditionalClauseInsertBehavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type should probably be renamed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the note, we agreed not to go with MapInsertionPath
at all
@@ -2318,6 +2318,15 @@ export const MetadataUtils = { | |||
const element = MetadataUtils.findElementByElementPath(metadata, target) | |||
return MetadataUtils.isJSXMapExpressionFromMetadata(element) | |||
}, | |||
isJSXElementFromMetadata(element: ElementInstanceMetadata | null): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised these two don't already exist, though we do have getJSXElementFromMetadata
and getJSXElementFromElementInstanceMetadata
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I followed the isConditionalFromMetadata
, isJSXMapExpressionFromMetadata
, etc pattern here - should I leave it like this (for completion and for the next developer who will need it and look for it) or do you think I need to use the getJSXElement...
methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so maybe we should leave this method for completion and for next developers, if that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, keep the function 👍
@@ -1228,3 +1232,7 @@ export function multiplePathsAllWithTheSameUID(paths: Array<ElementPath>): boole | |||
} | |||
return false | |||
} | |||
|
|||
export function isIndexedElement(path: ElementPath): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, we have isDynamicPath
which does the same thing but in a different way. I'm not sure which would be better now tbh
Problem:
We cannot currently wrap with a List
Fix:
This PR fixes wrapping a single element with a list.
Details:
INSERT
s in disguise) will actuallyREPLACE
the map contentsFor multiple element wrapping see #5696
Monosnap.screencast.2024-05-22.19-27-29.mp4
Fixes #5419