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

Grid component support #6640

Merged
merged 15 commits into from
Nov 18, 2024
Merged

Grid component support #6640

merged 15 commits into from
Nov 18, 2024

Conversation

gbalint
Copy link
Contributor

@gbalint gbalint commented Nov 14, 2024

Problem:
Followup to #6636

The goal is to support interaction with grid item in the following scenario:

  • the grid is in a component
  • the grid renders the component's children
  • the grid is not the root element of the component

Fix:
In #6636 I introduced a way to indentify a grid using the element path of its item (which is necessary because when the grid is in a non-focused component).

  • GridMeasurementHelper collects the grids from the metadata. These grids in non-focused components are not in the metadata, so they are not collected. I fixed this by collecting component items from the grids too.
  • To get the measurement data for these grids which identified by their items, I added a new parameter to getGridMeasurementHelperData so it can get the styling of the parentElement of the item's element.
  • The measurement helper was identified by its element path, and the dom-sampler could retrieve the helper from the dom using that. However, this is again not a good solution for these grids in non-focused components, so I added global WeakMap which stores the pairing between HTMLElements of grids and the id attribute of their GridMeasurementHelper.
  • I updated the path fiels names of GridContainerIdentifier and GridItemIdentifier: it was not a good idea to use the same name in the two paths, because it was possible to not check the type and use the path field.

Missing:
gridMoveStrategiesExtraCommands still tries to set grid props on the parent, and when it is a component, those can be absolutely useless. We should detect when that is the case, and not generate these commands.

Manual Tests:
I hereby swear that:

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

Copy link
Contributor

github-actions bot commented Nov 14, 2024

Try me

Copy link

relativeci bot commented Nov 14, 2024

#15164 Bundle Size — 58.08MiB (~+0.01%).

f687a58(current) vs d1d46c1 master#15154(baseline)

Warning

Bundle contains 70 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
#15164
     Baseline
#15154
Regression  Initial JS 41.06MiB(~+0.01%) 41.06MiB
No change  Initial CSS 0B 0B
Change  Cache Invalidation 18.14% 21.29%
No change  Chunks 20 20
No change  Assets 22 22
No change  Modules 4169 4169
No change  Duplicate Modules 213 213
Change  Duplicate Code 27.29%(-0.04%) 27.3%
No change  Packages 477 477
No change  Duplicate Packages 70 70
Bundle size by type  Change 2 changes Regression 1 regression Improvement 1 improvement
                 Current
#15164
     Baseline
#15154
Regression  JS 58.08MiB (~+0.01%) 58.07MiB
Improvement  HTML 7.37KiB (-0.25%) 7.39KiB

Bundle analysis reportBranch chore/grid-component-support-2Project dashboard


Generated by RelativeCIDocumentationReport issue

@gbalint gbalint changed the title Grid component support 2 Grid component support Nov 15, 2024
@gbalint gbalint marked this pull request as ready for review November 15, 2024 14:45
@@ -90,13 +96,17 @@ export type GridMeasurementHelperData = {
rowGap: number | null
columnGap: number | null
padding: Sides
element: HTMLElement
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing around and holding onto DOM elements makes me a little twitchy. It can shift around underneath us as React updates it and we're holding onto a reference to something which could hold onto a lot of memory potentially.

@gbalint gbalint merged commit 85df996 into master Nov 18, 2024
18 checks passed
@gbalint gbalint deleted the chore/grid-component-support-2 branch November 18, 2024 09:53
seanparsons pushed a commit that referenced this pull request Nov 18, 2024
**Problem:**
Followup to #6636

The goal is to support interaction with grid item in the following
scenario:
- the grid is in a component
- the grid renders the component's children
- the grid is not the root element of the component

**Fix:**
In #6636 I introduced a
way to indentify a grid using the element path of its item (which is
necessary because when the grid is in a non-focused component).

- `GridMeasurementHelper` collects the grids from the metadata. These
grids in non-focused components are not in the metadata, so they are not
collected. I fixed this by collecting component items from the grids
too.
- To get the measurement data for these grids which identified by their
items, I added a new parameter to `getGridMeasurementHelperData` so it
can get the styling of the parentElement of the item's element.
- The measurement helper was identified by its element path, and the
dom-sampler could retrieve the helper from the dom using that. However,
this is again not a good solution for these grids in non-focused
components, so I added global WeakMap which stores the pairing between
HTMLElements of grids and the id attribute of their
GridMeasurementHelper.
- I updated the `path` fiels names of `GridContainerIdentifier` and
`GridItemIdentifier`: it was not a good idea to use the same name in the
two paths, because it was possible to not check the type and use the
path field.

**Missing:**
`gridMoveStrategiesExtraCommands` still tries to set grid props on the
parent, and when it is a component, those can be absolutely useless. We
should detect when that is the case, and not generate these commands.

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
liady pushed a commit that referenced this pull request Dec 13, 2024
**Problem:**
Followup to #6636

The goal is to support interaction with grid item in the following
scenario:
- the grid is in a component
- the grid renders the component's children
- the grid is not the root element of the component

**Fix:**
In #6636 I introduced a
way to indentify a grid using the element path of its item (which is
necessary because when the grid is in a non-focused component).

- `GridMeasurementHelper` collects the grids from the metadata. These
grids in non-focused components are not in the metadata, so they are not
collected. I fixed this by collecting component items from the grids
too.
- To get the measurement data for these grids which identified by their
items, I added a new parameter to `getGridMeasurementHelperData` so it
can get the styling of the parentElement of the item's element.
- The measurement helper was identified by its element path, and the
dom-sampler could retrieve the helper from the dom using that. However,
this is again not a good solution for these grids in non-focused
components, so I added global WeakMap which stores the pairing between
HTMLElements of grids and the id attribute of their
GridMeasurementHelper.
- I updated the `path` fiels names of `GridContainerIdentifier` and
`GridItemIdentifier`: it was not a good idea to use the same name in the
two paths, because it was possible to not check the type and use the
path field.

**Missing:**
`gridMoveStrategiesExtraCommands` still tries to set grid props on the
parent, and when it is a component, those can be absolutely useless. We
should detect when that is the case, and not generate these commands.

**Manual Tests:**
I hereby swear that:

- [x] I opened a hydrogen project and it loaded
- [x] I could navigate to various routes in Play mode
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.

4 participants