Pin button in gridstack items

[hbcarlos]

Solves #110

Seams that the locked option is not working perfectly. When an item is pinned we can't move/resize it either.

Questions:

• How should we indicate the item is pinned or not? 🤔
• Should we modify the metadata format to save this property on disk?

[hbcarlos]

Metadata format changes with this PR, notebooks without attribute locked in metadata will be reset if they are open with the editor. This is also the reason why the test fails.

I'm not sure if this is the right way of attaching the toolbar:

voila-gridstack/packages/jupyterlab-gridstack/src/editor/gridstack/item.tsx

Line 42 in 83bf90e

onAfterAttach(): void {

The latest changes look like this:

[fcollonval]

Nice work!

I'm not sure if this is the right way of attaching the toolbar

I would suggest taking a approach similar to the file browser using a create function.

https://github.com/jupyterlab/jupyterlab/blob/98ccafa1e1b188e852fabb581d5d3f6aad38020e/packages/filebrowser/src/listing.ts#L216

https://github.com/jupyterlab/jupyterlab/blob/98ccafa1e1b188e852fabb581d5d3f6aad38020e/packages/filebrowser/src/listing.ts#L1761

It can be nice to set the grid item content as Panel rather than HTML node and add two children: the toolbar and the cell.

You can add a SingletonLayout on GridStackItem and add to it the panel.

[hbcarlos]

Thanks for the advice!

I'm not sure how to implement the layout.

[hbcarlos]

On my last PR, I created a model for items and added signals to communicate changes to the grid.

The idea would be to do a refactor to improve the items and reduce the number of signals for items in GridStackModel, creating a single signal to track changes on items changing the cellRemoved signal for something like this:

voila-gridstack/packages/jupyterlab-gridstack/src/editor/gridstack/model.ts

Line 62 in 01bcd2f

this._cellRemoved = new Signal<this, string>(this);

enum ItemState {
  CLOSED,
  LOCKED,
  UNLOCKED
}

interface ItemChange {
   item: GridStackItemModel;
   change: ItemState;
}

this._itemStateChanged = new Signal<this, ItemChange>(this);

What are your thoughts? Feedback is welcome!

[fcollonval]

Thanks for pushing on this one @hbcarlos

It looks almost good. I have a small concern about the item model life cycle and its connection to the toolbar rendering.

[fcollonval]

This can result in memory leak because you do not control the model life cycle.

It may be better to create the model within the widget and to trigger the model destruction when the widget is destroyed (and the signals need to be disconnected).

[fcollonval]

To be more robust, I would trigger the update if the model state changes. So in case in the future the model changes outside of the toolbar, the toolbar will react.

[fcollonval]

I went through the latest changes @hbcarlos
I have minor comments that should be easy to tackle.

[fcollonval]

You should avoid hard coding styles (bad practice). Instead you should add a common class to all buttons and set the svg size using that class and CSS rules.

[fcollonval]

It is better to use meaningful html tag; in this case button instead of div.

[hbcarlos]

Hey, thanks for the review! I'll change today.

[fcollonval]

Thanks @hbcarlos LGTM