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.

Sign up for GitHub

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

Jump to bottom

Convert to grid #6097

Merged
merged 19 commits into from
Jul 24, 2024
Merged

Convert to grid #6097

merged 19 commits into from
Jul 24, 2024

Conversation

bkrmendy
Copy link
Contributor

@bkrmendy bkrmendy commented Jul 18, 2024 •
edited
Loading

Description

This PR adds a basic convert-to-grid strategy, makes the flex <-> grid toggle work again, and turns the layout system + button into a menu-on-button where on can pick between grid or flex

Details/breakdown

  • add the convert-to-grid strategy (and a remove grid strategy)
  • make the flex <-> grid toggle work again
  • add the menu-on-button to choose between grid and flex
  • add a shortcut that turns a layout into a grid
  • expose a prop on the radix-based dropdown to align the dropdown element relative to the opener
  • update tests

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 18, 2024 •
edited
Loading

Try me

@relativeci RelativeCI
Copy link

relativeci bot commented Jul 18, 2024 •
edited
Loading

#13506 Bundle Size — 62.63MiB (+0.03%).

0875c97(current) vs 6339ece master#13497(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 4 changes Regression 2 regressions
                 Current
#13506		      Baseline
#13497
Regression  Initial JS 45.7MiB(+0.04%) 45.68MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 22.18% 21.51%
No change  Chunks 31 31
No change  Assets 34 34
Change  Modules 4380(+0.09%) 4376
Regression  Duplicate Modules 525(+0.38%) 523
No change  Duplicate Code 31.65% 31.65%
No change  Packages 469 469
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#13506		      Baseline
#13497
Regression  JS 62.62MiB (+0.03%) 62.6MiB
Improvement  HTML 11.16KiB (-0.33%) 11.2KiB

Bundle analysis report Branch feature/convert-to-grid Project dashboard

ruggi
ruggi reviewed Jul 24, 2024
View reviewed changes
editor/src/components/common/shared-strategies/convert-to-grid-strategy.ts
}
}

function guessMatchingGridSetup(children: Array<CanvasFrameAndTarget>): {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should also have a logic branch so that it makes sure that the combined number of resulting cells is >= than the number of children. For example if you have N children resulting from duplication, all having the exact same coordinates, the resulting grid template would be incorrect (e.g. 1x1 instead of 1xN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed on a side channel, I'll implement this on a separate PR because that way we'll have a holistic overview of the convert-to-grid strategy

Copy link
Contributor

Choose a reason for hiding this comment

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

keeping this unresolved so we can find it quickly later

@bkrmendy bkrmendy changed the title Feature/convert-to-grid Convert to grid Jul 24, 2024
@bkrmendy bkrmendy marked this pull request as ready for review July 24, 2024 14:01
@bkrmendy bkrmendy requested a review from balazsbajorics July 24, 2024 14:52
Rheeseyb
Rheeseyb reviewed Jul 24, 2024
View reviewed changes
editor/src/components/inspector/flex-section.tsx
const detectedLayoutSystems = selectedViews.map(
(path) =>
MetadataUtils.findElementByElementPath(metadata, path)?.specialSizeMeasurements
.layoutSystemForChildren ?? null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this return 'none' rather than null? I'm genuinely unsure

Copy link
Contributor Author

@bkrmendy bkrmendy Jul 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Completely fair question, I added the ?? null to remove the | undefined from the type of specialSizeMeasurements.layoutSystemForChildren to make it a less noisy type. I didn't add 'none' to make the type of detectedLayoutSystems similar to what we'd get if we checked specialSizeMeasurements.layoutSystemForChildren for a single element. The default value might as well be 'none' (it wouldn't matter for the downstream checks), but I didn't want to get creative and wanted to stick to the "spirit" of specialSizeMeasurements.layoutSystemForChildren

@bkrmendy bkrmendy merged commit 36f0e1c into master Jul 24, 2024
22 checks passed
@bkrmendy bkrmendy deleted the feature/convert-to-grid branch July 24, 2024 15:10
@ruggi ruggi mentioned this pull request Jul 29, 2024
Merged
ruggi added a commit that referenced this pull request Jul 29, 2024
@ruggi
Remove grid children props when removing layout (#6138)
4c44415 
Followup to #6097

**Problem:**
When removing a grid layout, its children still contain grid-specific
props after being converted to absolute, which can cause all sorts of
unpredictable/unintuitive issues.

**Fix:**

Prune those props.

Fixes #6137
liady pushed a commit that referenced this pull request Dec 13, 2024
@bkrmendy @ruggi
@lankaukk @Rheeseyb @liady
Convert to grid (#6097)
7a8a062 
## Description
This PR adds a basic convert-to-grid strategy, makes the flex <-> grid
toggle work again, and turns the layout system + button into a
menu-on-button where on can pick between grid or flex

### Details/breakdown
- add the convert-to-grid strategy (and a remove grid strategy)
- make the flex <-> grid toggle work again
- add the menu-on-button to choose between grid and flex
- add a shortcut that turns a layout into a grid
- expose a prop on the radix-based dropdown to align the dropdown
element relative to the opener
- update tests

---------

Co-authored-by: Federico Ruggi <[email protected]>
Co-authored-by: McKayla Lankau <[email protected]>
Co-authored-by: RheeseyB <[email protected]>
liady pushed a commit that referenced this pull request Dec 13, 2024
@ruggi @liady
Remove grid children props when removing layout (#6138)
056ce08 
Followup to #6097

**Problem:**
When removing a grid layout, its children still contain grid-specific
props after being converted to absolute, which can cause all sorts of
unpredictable/unintuitive issues.

**Fix:**

Prune those props.

Fixes #6137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruggi ruggi ruggi approved these changes

@liady liady liady approved these changes

@balazsbajorics balazsbajorics balazsbajorics approved these changes

@Rheeseyb Rheeseyb Rheeseyb approved these changes

@gbalint gbalint Awaiting requested review from gbalint

@seanparsons seanparsons Awaiting requested review from seanparsons

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bkrmendy @Rheeseyb @ruggi @balazsbajorics @liady @lankaukk