-
Notifications
You must be signed in to change notification settings - Fork 174
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
Grid component support #6640
Conversation
#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
Bundle size by type
Bundle analysis report Branch chore/grid-component-support-2 Project dashboard Generated by RelativeCI Documentation Report issue |
@@ -90,13 +96,17 @@ export type GridMeasurementHelperData = { | |||
rowGap: number | null | |||
columnGap: number | null | |||
padding: Sides | |||
element: HTMLElement |
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.
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.
**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
**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
Problem:
Followup to #6636
The goal is to support interaction with grid item in the following scenario:
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.getGridMeasurementHelperData
so it can get the styling of the parentElement of the item's element.path
fiels names ofGridContainerIdentifier
andGridItemIdentifier
: 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: