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

Simplify react lib design #2938

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Simplify react lib design #2938

wants to merge 5 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.

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.

2 participants