Update GridStack when item options change

[btecu]

Closes #61.

[jkusa]

@btecu thanks for your work on this issue!

Can we add some testing around this bug?

[btecu]

@jkusa this is related to the internal state of the GridStack. Any ideas on how to test that?

[btecu]

Looks like existing hooks (https://github.com/yahoo/ember-gridstack/blob/master/addon/components/grid-stack-item.js#L79) that interact with the GridStack instance are not tested. I'm guessing there might not be a way.

[jkusa]

@btecu what behaviors did you observe before and after your change?

[btecu]

@jkusa I've added examples on the ticket related issue (#61).

[btecu]

@balajimanoharan @jkusa what's the status here?

[balajimanoharan]

Sorry about the delay. Im ok with merging this now, while we figure out a way to test gridstack interactions. @jkusa do you still have concerns?

[jkusa]

@btecu yes sorry for the delay. Could you use your ember-grid-bug example as a basis for a test? After updating the grid-stack-item, assert the data-gs-* attributes.

[btecu]

@jkusa test added.

[jkusa]

@btecu thanks for adding the test. Unfortunately, it looks like the test passes even without your change.

[btecu]

@jkusa As I wrote earlier, the issue is related to some internal state of GridStack.

[kevinhinterlong]

@btecu it looks like from your example that without this method, the onChange method is never called, but after the change it is.

So you should be able to test this by making something like

assert.expect(1);

this.set('onChange', () => assert.ok(true, 'onChange was called after changing GridStackItem options'));

this.set('options', { x: 0, y: 0, width: 5, height: 1 });

await render(hbs`
  <GridStack @onChange={{this.onChange}}>
      <GridStackItem @options={{this.options}}>
        Test
      </GridStackItem>
  </GridStack>
`);

this.set('options', { x: 2, y: 1, width: 12, height: 1 });

[kevinhinterlong]

Suggested change

	let parent = this.parentContainer;
	const parent = this.parentContainer;

[kevinhinterlong]

Suggested change

	let { height, width, x, y } = this.options;
	const { height, width, x, y } = this.options;

[jkusa]

Hey @btecu, regarding:

@jkusa As I wrote earlier, the issue is related to some internal state of GridStack.

I get that. From your demo app & GIFs the internal state issue manifests into an undesired behavior at the client level correct? If you can (visually) observe the change, can we not assert the correct behavior with an acceptance or integration test?

[btecu]

@jkusa I've updated the test to check whether onChange get's called, since it seems a bit cleaner than visual checks.

[jkusa]

If we do this for height, width, x, y do we need to keep attributeBindings for height, width, x, y?

[btecu]

I'm not sure there is any benefit in adding an exclusion list. It would be nice to remove the attributeBindings completely, since they are not supported in glimmer components.

[jkusa]

I think the benefit would be not updating the DOM multiple times (with update() & with attributeBindings) when grid-stack-item args update. It also would be nice to have a single path to set x, y, width, height (and all args).

It would be nice to remove the attributeBindings

Totally agree 👍

I spoke with a gridstack maintainer on slack, looks like direct attribute updates are not officially supported. I filed an issue for this enhancement. Another issue is that not all item options are supported in the current update() method. I filed an issue for that too :).

Given, the need for a larger refactor, I think we can merge this and clean up the flow separately.

Thanks for your work on this 👍

[adumesny]

I can fix the GridStack.update() to take all options (easy) but not sure I will get to (or should) track DOM changes to update internal state.