contentful / experience-builder

https://www.contentful.com/developers/docs/experiences/what-are-experiences/
MIT License
6 stars 1 forks source link

refactor: [SPA-1474] move selected state to context #123

Closed loweisz closed 8 months ago

loweisz commented 8 months ago

To have one dedicated place to store the information what is the currently selecteded node id, I have added a context to store this in one place. This should fix the issue of the non updated selection outline when changing something in the component as they are now updated correctly.

https://github.com/contentful/experience-builder/assets/34910067/3fd561a7-7596-4ac5-98b6-33c7e7e1364b

Note: This approach is not framework agnostic, though I still decided to go that way as we are rendering the whole tree in a react way at the moment, so this is currently the safest way to store a global state. If we decide to make the sdk completly sdk free we need to change the rendering mechanism which also allows us to add a different way of handling global state.

Spring3 commented 8 months ago

@loweisz just curious - what is the main change which solved the issue? Before we had one hook where the state was stored. Now we have 1 context where the state is stored. I think theoretically both ways should behave the same way (I am yet to see the changes in the PR) - regardless whether the setState() happens inside of the context or inside of the hook

Curious whether introducing the context was really necessary in this case

loweisz commented 8 months ago

@Spring3 Essentially the issue was that only set the state in the hook in one place when it came from the web app by clicking in the layers tab. That led to it not being selected in the state when you click on the element in the canvas, so the update was not triggered. https://github.com/contentful/experience-builder/pull/123/files#diff-515c27014ce1336cc651954a8dd2bd7fceff1fd5513f891f17c9b53cec019b99R188

I solved this issue by adding it to the context so we always have the right selected element in the state.

Spring3 commented 8 months ago

@Spring3 That led to it not being selected in the state when you click on the element in the canvas, so the update was not triggered. https://github.com/contentful/experience-builder/pull/123/files#diff-515c27014ce1336cc651954a8dd2bd7fceff1fd5513f891f17c9b53cec019b99R188

not related to the changes in this PR, but do you know what could be the reason why, after setState() in the hook, the value in the callback had a wrong value? Just looking at the code, it is passed as prop and then included everywhere as a dependency, so it should work correctly

I am just trying to understand what went wrong. Do you have an idea?

loweisz commented 8 months ago

It would be 💯 if you could add a test to cover this change

Works when I tested it locally

It's not super easy to actually test the size changes due to the jsdom. I've just tested the resending of the element

contentful-automation[bot] commented 8 months ago

:tada: This PR is included in version 1.1.0-alpha.75 :tada:

The release is available on:

Your semantic-release bot :package::rocket: