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: Wrap multiple elements using the new picker #5758

Merged
merged 4 commits into from
May 28, 2024
Merged

Conversation

liady
Copy link
Contributor

@liady liady commented May 27, 2024

Problem:
Our current picker does not support multiple selections, we need this functionality for wrapping multiple elements

Fix:
Make the new picker accept multiple selections.

Important note: this PR adds the functionality of wrapping multiple elements, but for replacment and inserts the target remains only the first selection (as it was before).
We need to decide later on what to do about insertion/replacements

Monosnap.screencast.2024-05-27.17-26-52.mp4

Manual Tests:
I hereby swear that:

  • I opened a hydrogen project and it loaded
  • I could navigate to various routes in Preview mode

Fixes #5696

Copy link
Contributor

github-actions bot commented May 27, 2024

Try me

Copy link

relativeci bot commented May 27, 2024

#12668 Bundle Size — 62.24MiB (~+0.01%).

454d644(current) vs 6339ece master#12658(baseline)

Warning

Bundle contains 51 duplicate packages – View duplicate packages

Bundle metrics  Change 2 changes Regression 1 regression
                 Current
#12668
     Baseline
#12658
Regression  Initial JS 45.31MiB(~+0.01%) 45.31MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 22.07% 22.28%
No change  Chunks 30 30
No change  Assets 33 33
No change  Modules 4292 4292
No change  Duplicate Modules 521 521
No change  Duplicate Code 31.78% 31.78%
No change  Packages 448 448
No change  Duplicate Packages 51 51
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#12668
     Baseline
#12658
Regression  JS 62.23MiB (~+0.01%) 62.23MiB
Improvement  HTML 11.16KiB (-0.33%) 11.2KiB

Bundle analysis reportBranch feat/wrap-multipleProject dashboard

Copy link
Contributor

github-actions bot commented May 27, 2024

Performance test results:
(Chart1)
(Chart2)

@liady liady marked this pull request as ready for review May 27, 2024 19:15
@balazsbajorics
Copy link
Contributor

I've left a message on discord about special casing some wrap behaviors for div, fragment and group

Copy link
Contributor

@balazsbajorics balazsbajorics left a comment

Choose a reason for hiding this comment

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

we should not forget about re adding the special cased wrapping behavior for div, group, fragment

Copy link
Contributor

@seanparsons seanparsons left a comment

Choose a reason for hiding this comment

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

Better to avoid those subscripts because they're a bug waiting to happen.

@liady liady merged commit e29bcac into master May 28, 2024
14 checks passed
@liady liady deleted the feat/wrap-multiple branch May 28, 2024 11:35
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.

Extend wrap functionality
4 participants