-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
base: master
Are you sure you want to change the base?
Conversation
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"; |
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.
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 { |
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.
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.
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.
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
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.
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.
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.
Got it.
I haven't specifically focused on the darg&drop between grid issue, but I find it works well in this pr demo.
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 |
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. |
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 :
I started out designing one component like
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 (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>
);
} |
| 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 | why having to maintain external state ? gridstack dom elements already have a | in the angular case I use 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 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 |
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.
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.
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.
LOL. That's what I'm struggling with, I'm finding places in the code where it's
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. |
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 :) |
| It's more like actions like save / restore require access to instance
| 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, @Component({
selector: 'gridstack',
...
export class GridstackComponent implements OnInit, AfterContentInit, OnDestroy { | I think selector + input that's a good idea. 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 |
My expression was inaccurate, I meant to say that gridstack defines the option data type well.
What about considering new fields? Like |
| 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. |
Description
Checklist
yarn test
)