Object properties panel #20
Conversation
martinRenou
commented
Jul 1, 2024
•
martinRenou
commented
|
|
| localState && | ||
| localState.selected?.emitter && | ||
| localState.selected.emitter !== this.state.id && | ||
| localState.selected?.value |
There was a problem hiding this comment.
Can't the selected value be undefined if there is no selection ?
There was a problem hiding this comment.
Yes it can, do you have a concern about this if condition?
There was a problem hiding this comment.
I might be missing something, but my understanding is:
localState.selected?.value is undefined -> newState is not updated -> the setState is not triggered -> the panel is not updated
But we probably want to update it to an empty panel since there is no selection.
| @@ -45,7 +45,13 @@ export interface IJGISSourceDocChange { | |||
| }>; | |||
| } | |||
|
|
|||
| export interface ISelection { | |||
| type: 'layer' | 'source'; | |||
There was a problem hiding this comment.
Can you think of use cases where we can select both layers and sources?
There was a problem hiding this comment.
You mean selecting both layers and sources at the same time?
Actually I think I'm not using the type entry anymore in my implementation, I can remove it.
There was a problem hiding this comment.
You mean selecting both layers and sources at the same time?
Yes, this is what I meant.
We will probably want to be able to select several sources or several layers (to delete them, or maybe update properties).
But I don't know if we want to select layers and sources at the same time, this is why I wonder if this type is need.
There was a problem hiding this comment.
This entry was just some metadata information on the selection (we could add more metadata if we want in the future), but we can already infer from the id of the selected object if it's a layer or a source. So I should probably remove it.
So technically my code allows selecting both layers and sources at the same time. We'll see if we actually need this if we have use cases.
There was a problem hiding this comment.
Looks good, thanks for the clarification
