Skip to content

Simplify react lib design #2938

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Simplify react lib design #2938

wants to merge 8 commits into from

Conversation

Aysnine
Copy link
Contributor

@Aysnine Aysnine commented Feb 7, 2025

Description

  • Simple demo that use id selector
  • Advanced demo with custom dynamic components

Checklist

  • Created tests which fail without the change (if possible)
  • All tests passing (yarn test)
  • Extended the README / documentation, if necessary

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 7, 2025

function App() {
  const [uncontrolledInitialOptions] = useState<GridStackOptions>({
    // ...
    children: [
      { id: "item1", h: 2, w: 2, x: 0, y: 0 },
      { id: "item2", h: 2, w: 2, x: 2, y: 0 },
    ],
  });

  return (
    <GridStackProvider initialOptions={uncontrolledInitialOptions}>
      <Toolbar />

      <GridStackRender>
        <GridStackItem id="item1">
          <div>hello</div>
        </GridStackItem>

        <GridStackItem id="item2">
          <div>grid</div>
        </GridStackItem>
      </GridStackRender>
    </GridStackProvider>
  );
}

@@ -1,35 +0,0 @@
import type { GridStack, GridStackOptions, GridStackWidget } from "gridstack";
Copy link
Member

Choose a reason for hiding this comment

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

hey don't delete those files. I actually wanted to start shipping them like I do for /angular
I started looking at the code and was planning to do this weekend... I wanted to match better what I did for angular wrapper (extending WidgetItem to have similar to 'selector' + 'input' which are angular centric but conceptually the same as name + props you have.

@@ -0,0 +1,33 @@
import type {
Copy link
Member

Choose a reason for hiding this comment

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

oh I see you moved code here... quite different. will have to take a look so we can make this maybe official soon and publish as NPM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we need to agree on the location of the libs, I didn't check how the wrappers for the other frameworks were packaged, so I temporarily moved the folders to make it easier for me to copy the complete code between multiple demo codebases

Copy link
Member

Choose a reason for hiding this comment

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

exact location doesn't matter. I just need to publish those as part of the NPM package so users can use the classes directly and get version fixes... angular happens to use angular\projects\lib\src\lib but what you have is fine.

we need to try and match angular wrapper syntax and simplicity whiel fully supporting nested and custom component inside grid items (gs cna handle the containers) and allow darg&drop between grid without re-creating the expensive content (you app components). that's what I did for Angular.

thank you for creating React version as I know it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

I haven't specifically focused on the darg&drop between grid issue, but I find it works well in this pr demo.

https://gridstack-react.pages.dev/

@adumesny
Copy link
Member

adumesny commented Feb 7, 2025

if you look at the angular version, this is what I have

<gridstack [options]="gridOptions"></gridstack>

Is there a reason React can't match exactly ? the above is very simple DOM and provides the FULL power of GS as it doesn't try to create DOM children directly (and sync states between 2 frameworks) ?

in the DOM 'simple way (not recommended) that closer to your simple case above I also support

<gridstack [options]="gridOptions" (changeCB)="onChange($event)">
   @for (n of items; track n.id) {
    <gridstack-item [options]="n">Item {{n.id}}</gridstack-item>
  }
</gridstack>

so I have <gridstack> and <gridstack-item> components.

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 7, 2025

I don't have experience with angular so I didn't check its example. But it seems necessary to learn angular examples, I'll go back and confirm.

@Aysnine Aysnine marked this pull request as draft February 8, 2025 02:34
@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 8, 2025

I'm gonna set up this pr as a draft. We can align the lib design here.

I compared angular lib components with my react lib components. I found that the responsibilities of my components are much more finely divided.

In fact :

  • angular <gridstack> = my react <GridStackProvider> + <GridStackRender>
  • angular <gridstack-item> = my react <GridStackItem>

I started out designing one component like angular <gridstack>, but considering cases like Toolbar, which doesn't want to be in the gridstack container dom but needs to access the gridstack instance, I separated the responsibilities of one component into two:

  • <GridStackProvider> only manages gridstack instances, doesn't output dom, and provides gridstack instances to internal scopes via react context.
  • <GridStackRender> outputs a root dom and stores containers from the gridstack renderCB so that the <GridStackItem> of the internal scope can mount the contents.

When designing react components, my rule is that everything is component-first, and I think about delineating component responsibilities as much as possible.

Maybe I could design a <GridStackContainer> = <GridStackProvider> + <GridStackRender> ?

(The reason for not using directly but is to avoid naming conflicts with 'type GridStack'.)

The result looks like this:

function App() {
  const [uncontrolledInitialOptions] = useState<GridStackOptions>({
    // ...
    children: [
      { id: "item1", h: 2, w: 2, x: 0, y: 0 },
      { id: "item2", h: 2, w: 2, x: 2, y: 0 },
    ],
  });

  return (
    <GridStackContainer initialOptions={uncontrolledInitialOptions}>
      <GridStackItem id="item1">
        <div>hello</div>
      </GridStackItem>

      <GridStackItem id="item2">
        <div>grid</div>
      </GridStackItem>
    </GridStackRender>
  );
}

@adumesny
Copy link
Member

adumesny commented Feb 8, 2025

| Toolbar, which doesn't want to be in the gridstack container dom but needs to access the gridstack instance

why ? the gristack init params says if it accepts external widgets (add enter/drop events). then STATIC GridStack.setupDragIn() is used to init toolbar (adds drag events), but it doesn't need to access the instance (there could be multiple). see two.html

| <GridStackProvider> only manages gridstack instances

why having to maintain external state ? gridstack dom elements already have a gridstack pointer (and item GridItemHTMLElement have gridstackNode) and in the angular case I also store the Angular component pointers _gridComp | _gridItemComp. so from Class or DOM I can access either one. When DOM get destroyed, those are also gone - see ngOnDestroy

| <GridStackRender> outputs a root dom and stores containers from the gridstack renderCB

in the angular case I use addRemoveCB (see gsCreateNgComponents) rather than the lower renderCB because angular component (your app children) can only live inside other angular components, so <gridstack> and <gridstack-item> are both angular components, but that's an angular restrictions and I'm guessing React can be hosted inside any plain DOM. So yeah I added renderCB exactly for that case...

you can at the very least use the same dom name and [options] so the code looks mostly the same. We also need to talk about how custom comp are in the JSON. the old content string was legacy stuff. In angular I subclasses and added 2 params selector + input (very angular term, the second matching component @Input vs @Output fields for params). Thinking I may want to make content be a custom type (liek generic 'data' typically is) that default to string for legacy but can be our JSON app framework field of choices or something...

and THANK YOU for working with me on a native React wrapper as I always wanted to get this out but don't use/know React

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 8, 2025

why ? the gristack init params says if it accepts external widgets (add enter/drop events). then STATIC GridStack.setupDragIn() is used to init toolbar (adds drag events), but it doesn't need to access the instance (there could be multiple). see two.html

My example here may not be appropriate. It's more like actions like save / restore require access to instance.

The toobar I have here is not quite like the one in two.html, it's more like it's made purely to give an example of how to access the gridstack through the react context. the toolbar in two.html I haven't tried yet, it's the one that's missing from this pr, I'll follow up on that.

why having to maintain external state ? gridstack dom elements already have a gridstack pointer (and item GridItemHTMLElement have gridstackNode) and in the angular case I also store the Angular component pointers _gridComp | _gridItemComp. so from Class or DOM I can access either one. When DOM get destroyed, those are also gone - see ngOnDestroy

As I follow the component-first principle (instead of dom-first), the GridStackProvider doesn't just store the state, but also takes care of handling its lifecycle. And it provides a more standard react context way for other components to access the gridstack instance instead of getting it from the dom.

in the angular case I use addRemoveCB (see gsCreateNgComponents) rather than the lower renderCB because angular component (your app children) can only live inside other angular components, so and are both angular components, but that's an angular restrictions and I'm guessing React can be hosted inside any plain DOM. So yeah I added renderCB exactly for that case...

You've reminded me that I haven't focused on ways other than renderCB, but renderCB is enough for me at the moment. I'll go back and check.

you can at the very least use the same dom name and [options] so the code looks mostly the same.

LOL. That's what I'm struggling with, I'm finding places in the code where it's <gridstack-xxx>, which is a word, and places where it's type GridStack. In some places it's type GridStack, two words. Since it's common in react to use big humps for components, and since word spelling is easy to recognize, I followed the name type GridStack.

We also need to talk about how custom comp are in the JSON. the old content string was legacy stuff. In angular I subclasses and added 2 params selector + input (very angular term, the second matching component @input vs @output fields for params). Thinking I may want to make content be a custom type (liek generic 'data' typically is) that default to string for legacy but can be our JSON app framework field of choices or something...

I think selector + input that's a good idea.

Also, in this pr, I removed the react implementation of JSON content from the lib to make it more pure. Putting the removed code implementation into advanced is an example of this.

If selector + input is implemented in gridstack, I just need to update my grid-stack-item component implementation for quick adaptation.

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 8, 2025

and THANK YOU for working with me on a native React wrapper as I always wanted to get this out but don't use/know React

gridstack provides very good support for custom panels for my in-house projects, and it's worth providing a better-working react wrapper for this purpose :)

@adumesny
Copy link
Member

adumesny commented Feb 8, 2025

| It's more like actions like save / restore require access to instance

GridStack.init() or GridStack.initAll() return instances, and angualr wrapper GridstackComponent stores it when doing this._grid = GridStack.init() so no need to keep a global list (one fewer thing to get out of sync). it's up to the app if they want to track <gridstack> which has an angular way of doing.

| As I follow the component-first principle

same with angular wrapper. all the app deals with is the ng components - their life cycle will destroy the DOM parts as needed.

| You've reminded me that I haven't focused on ways other than renderCB, but renderCB is enough for me at the moment. I'll go back and check.

renderCB is what you should use (or higher for Angular needs)

| at the very least use the same dom name and [options] so the code looks mostly the same.. That's what I'm struggling with, I'm finding places in the code where it's

GridStack is the TS class/type, <gridstack-xxx> is the DOM. the angular component looks this way (selector dom is as listed, but it's TS class has common Angular pattern and native.

@Component({
  selector: 'gridstack',
...
export class GridstackComponent implements OnInit, AfterContentInit, OnDestroy {

| I think selector + input that's a good idea.
| If selector + input is implemented in gridstack

isn't that odd to React ? (very angular component pattern). I wasn't planning to add those to GS itself (framework specific), but maybe make existing 'content' (which is generic similar to 'data') to be able to be { selector: string, input: T} for angular case, string for legacy html/JS, and possibly something more React for your case... that woudl be a breaking change for Ng so I have to think carefully on this. once we publish wrapper, it's like breaking API on GS itself.

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 9, 2025

If selector + input is implemented in gridstack

My expression was inaccurate, I meant to say that gridstack defines the option data type well.

maybe make existing 'content'

What about considering new fields? Like data or meta? That way you don't have to break the content.

@adumesny
Copy link
Member

adumesny commented Feb 9, 2025

| What about considering new fields? Like data or meta? That way you don't have to break the content.

yeah, maybe. content: string could also be the default type if there is a way to have that and still have a T for others.

@Aysnine
Copy link
Contributor Author

Aysnine commented Feb 28, 2025

@adumesny I just realized a serious bug where assigns a value to GridStack.renderCB, and when there are multiple 's, this causes GridStack.renderCB to be duplicated, resulting in a duplication of the id maintained in each Provider. element map maintained in each Provider is inaccurate.

Also, the id doesn't seem to be required for each item, which I hadn't considered.

@adumesny
Copy link
Member

internal ids have to be globaly unique (why it's a running static counter) as you can move a widget from one grid to another... user ids should be unique at least per grid so save/load can map to correct instance of a widget to update.

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 2, 2025

internal ids have to be globaly unique (why it's a running static counter) as you can move a widget from one grid to another... user ids should be unique at least per grid so save/load can map to correct instance of a widget to update.

Resolved.

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 2, 2025

Another confusion:

const instance = GridStack.init(optionsRef.current, containerRef.current);
instance.on("dropped", (e) => {
  console.log("dropped", e);
  // ! subGrid not working
});

All event not receive about subGrid.

I'm stuck here now because I want to listen for grid changes and dynamically render the react component in the react advanced example, which is a more common scenario.

@Aysnine Aysnine marked this pull request as ready for review March 2, 2025 02:41
@adumesny
Copy link
Member

adumesny commented Mar 2, 2025

All event not receive about subGrid.|

correct, subgrids do not currently propagate events to the top one (where you add the event handler). see #2671, which I could look into

I'm stuck here now because I want to listen for grid changes and dynamically render the react component in the react advanced example, which is a more common scenario.

when you move an item from one grid to another (child or not) you should NOT have to re-render the component (as creating them can be expensive, making API calls all over, etc... in the anular wrapper and let GS re-parent things and everything is OK BECAUSE I don't have Angualr create the dom fomr state, but let GS drive the state and callback to wrapper to create component when needed.

Are you not doing that ? that's a must change IMO.

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 2, 2025

when you move an item from one grid to another (child or not) you should NOT have to re-render the component (as creating them can be expensive, making API calls all over, etc... in the anular wrapper and let GS re-parent things and everything is OK BECAUSE I don't have Angualr create the dom fomr state, but let GS drive the state and callback to wrapper to create component when needed.

That's actually what I was thinking, but it's different with dragIn (clone), which always requires the creation of a new component.

And dragIn to subGrid can't be listened to. So I implemented the rendering of the dragIn copied component in a different way, instead of relying on listening. But the downside is that it's not good for save/load.

@adumesny
Copy link
Member

adumesny commented Mar 2, 2025

| it's different with dragIn (clone), which always requires the creation of a new component.

right, drag froms idebar is always a preview and actual comp needs to be created. instead it's cleaner, and what I have in angualr wrapper, to use the WidgetItem as what to create and use GridStack.renderCB or GridStack.addRemoveCB which is also used during reading, to create the right component bu storing custom info on what to create (angular 'selector' field). React should do the same.

I really want React and Angular wrapper doc to look and act the same. so again I suggest you study what I do and do the equicalent react code version if not totally weird for that framework. GS needs to maintain state and re-parent and resize things as it needs to, independ on the framework. the framework only job is to create the right element (or <gridstack> ` parent as angualr needs entire hierachy to be ng components) instead of plain divs with classes.

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 3, 2025

I really want React and Angular wrapper doc to look and act the same.

Yes. I want to run /angular folder. But error when install deps:

➜ gridstack.js/angular master ✓ yarn
yarn install v1.22.22
[1/4] 🔍  Resolving packages...
[2/4] 🚚  Fetching packages...
error Error: https://usw1.packages.broadcom.com/artifactory/api/npm/tis-npm-virtual/@angular/core/-/core-14.3.0.tgz: Request failed "403 Forbidden"
    at ResponseError.ExtendableBuiltin (/Users/jace/.cache/node/corepack/v1/yarn/1.22.22/lib/cli.js:696:66)
    at new ResponseError (/Users/jace/.cache/node/corepack/v1/yarn/1.22.22/lib/cli.js:802:124)
    at Request.<anonymous> (/Users/jace/.cache/node/corepack/v1/yarn/1.22.22/lib/cli.js:66750:16)
    at Request.emit (node:events:524:28)
    at module.exports.Request.onRequestResponse (/Users/jace/.cache/node/corepack/v1/yarn/1.22.22/lib/cli.js:142287:10)
    at ClientRequest.emit (node:events:524:28)
    at HTTPParser.parserOnIncomingClient (node:_http_client:716:27)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:117:17)
    at TLSSocket.socketOnData (node:_http_client:558:22)
    at TLSSocket.emit (node:events:524:28)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

Is this a package from a private registry?

@adumesny
Copy link
Member

adumesny commented Mar 4, 2025

| error Error: https://usw1.packages.broadcom.com/artifactory/api/npm/tis-npm-virtual/@angular/core/-/core-14.3.0.tgz

oops, sorry. let me fix that asap. yes, private repo... fixed #2977

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 4, 2025

@adumesny I have two questions about angular wrapper:

  1. Is comp1 re-render when move item from grid a to grid b ?
  2. What will be happen when move item from grid a to grid b and destroy grid a?

image

I'm wondering if the comp1 lifecycle goes with the grid, or with the dom ref, or something else.

I briefly looked at how to dynamically create components in angular, which is very different from react, which doesn't really create components based on a dom ref, but instead relies on the parent component to render itself and mounts the content to the dom ref, which looks like it creates a component, but its lifecycle is always affected by the parent and can't be customized.

@adumesny
Copy link
Member

adumesny commented Mar 4, 2025

  1. no delete/re-create. you can see that using the /angular demo (yarn + yarn start), pick two grid demo, and look at the log while dragging an object B to the other grid
    image

  2. will have to test this for sure (I can mod the example) but if you look at GridstackComponent you will see

  /** container to append items dynamically (recommended way) */
  @ViewChild('container', { read: ViewContainerRef, static: true}) public container?: ViewContainerRef;

angular can have dynamic list of children and I'm pretty sure when GS manually reparents a child the parent 'container' doesn't try to destroy an old child...

@Aysnine
Copy link
Contributor Author

Aysnine commented Mar 5, 2025

angular can have dynamic list of children and I'm pretty sure when GS manually reparents a child the parent 'container' doesn't try to destroy an old child...

I think this may be the essence of why react wrappers are not easy to design. react's rendering style dictates that item content cannot be designed as a react component that can be re-parented.

Same two questions but in react:

  1. no delete/re-create.
  2. item content (not item container by GS) will be destroy

That's why, I've seen previous GS react wrappers or examples attempting controlled components, not uncontrolled ones like mine. It's because they want to dispatch the react component with the help of the item meta in the options state to make sure the component looks re-parent.

I need to calm down a bit.

While it occurred to me that I could use react's multiple createRoots to get each item content component off of its parent, this is not very performant and goes against the principle of a single root for react.

@adumesny
Copy link
Member

ok, so how do you want to proceed ? in the end when a widget is moved from a grid to another it is best to not recreate the expensive content (could be long API calls) and let GS just do a DOM re-parenting. this doesn't break Angular wrapper as GridItem comp (which then contains actual user comp) are created dynamically and will update parents list of children for destroy.

if that's not possible in React then so be it.

@chungwong
Copy link

I gave the new design a spin but I didn't get far. Is it ready for testing?
The first error I am getting is getContainerByWidgetId is unimplemented yet

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.

3 participants