Store sampleview coords in context menu state #1489
Conversation
axelboc
force-pushed
the
ab-context-menu-2
branch
from
792d4ca
to
e649607
Compare
| @@ -486,7 +479,7 @@ export default class ContextMenu extends React.Component { | |||
| const menuOptions = this.menuOptions(); | |||
| let optionList = []; | |||
|
|
|||
| if (this.props.sampleID !== undefined) { | |||
There was a problem hiding this comment.
I've kept this condition for now, but I don't find it ideal. If there's no sample mounted and you right-click on a shape, you get a completely different context menu than if you right-click on the same shape and there's a sample mounted. Actions that don't require a sample to be mounted, like Delete Line, could very well be shown when right-clicking on a line regardless of the presence of a sample.
I would prefer for context menu actions that require a sample to be mounted to be conditionally rendered individually.
There was a problem hiding this comment.
I would prefer for context menu actions that require a sample to be mounted to be conditionally rendered individually or to show an error to the user when used.
Yes, the context menu on a shape without a sample mounted should give an error, just like we can't create a point/shape without a sample
There was a problem hiding this comment.
Sorry, I've edited my comment to clarify my thought: I think that actions that don't require a sample to be mounted, like Delete Point should be shown when right-clicking on a shape regardless of the presence of a sample.
That's because when you mount a sample, perform 3-click centring, save the created point, and unmount the sample, the point remains on the sample view, so you can still right-click on it. At the moment, when you're in this situation, you basically can't delete anything via the context menu (you still can by pressing Delete):
There was a problem hiding this comment.
Hm, yes makes sense from that "point" (no pun intended :) ) of view. But all the shapes should actually be cleared when a sample is unmounted, because they are not valid any longer. We should look at that if its not the case
There was a problem hiding this comment.
On the beamlines, at least ours, the shapes are removed at unmount, I guess its just a event that is not registered properly in some mockup.
| @@ -34,24 +33,6 @@ export default class ContextMenu extends React.Component { | |||
| grid: [], | |||
| none: [], | |||
| }; | |||
| let twoDPoints = []; | |||
|
|
|||
| if (this.props.enable2DPoints) { | |||
There was a problem hiding this comment.
I've inlined the condition below.
| ], | ||
| }; | ||
|
|
||
| if (this.props.enableNativeMesh) { |
There was a problem hiding this comment.
Also inlined above.
marcus-oscarsson
force-pushed
the
ab-context-menu-2
branch
from
e649607
to
156d948
Compare
|
Replacing the dummy shape |
axelboc
force-pushed
the
ab-context-menu-2
branch
from
156d948
to
682af00
Compare
|
I've used optional chaining in |
| const { sampleViewX, sampleViewY } = this.props; | ||
|
|
||
| this.props.sampleViewActions.add2DPoint( | ||
| sampleViewX, | ||
| sampleViewY, | ||
| 'SAVED', | ||
| (shape) => this.showModal(name, {}, shape, extraParams), |
There was a problem hiding this comment.
Maybe you already thought about it and want to do that in a different PR, but related to #1490: Couldn't we just prevent the drawing of a point when no sample is mounted by adding a check for sampleData (similar to how it is done in showModal)
There was a problem hiding this comment.
Indeed, the check for the mounted sample could just come before the creation of the point. But we could also hide or disable the context menu option.
There was a problem hiding this comment.
You are correct, that might be a more elegant solution to the problem. Sorry, It seems that I misunderstood the other comment, the first time I read it
marcus-oscarsson
force-pushed
the
ab-context-menu-2
branch
from
682af00
to
2542acb
Compare
I now store
e.offset<X|Y> / imageRatioinstead of juste.offset<X|Y>in the state, which savesContextMenufrom having to do this calculation in a couple of places.I've made a couple of additional commits to move some logic around, inline some methods, etc., and to let the right-clicked shape be
undefinedin the state (instead of storing a fake shape object{ type: 'NONE' }).