Skip to content

Conversation

@nafiz1001
Copy link
Collaborator

No description provided.

@nafiz1001 nafiz1001 requested a review from UlysseFG January 5, 2026 15:30
<Radio.Button value={PlacementDirections.COLUMN}>Column</Radio.Button>
</Radio.Group>
</Flex>
<ButtonsWithSmallerPadding>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe leave some padding ...The buttons are a bit too close ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i replied on the wrong comment. oops.

theme={{
components: {
Button: {
paddingInline: '0.5em',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we as much as possible use relative measures (ie %) for components and use absolute measures for text?

Copy link
Collaborator Author

@nafiz1001 nafiz1001 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

em is already relative. Relative to parent font size.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah but the padding of the button ... how much is it really related to font size ? Makes more sense to have it related to the parent container size.

<ButtonsWithSmallerPadding>
<Flex justify={"space-between"} style={{ width: "100%" }}>
<Space size={"small"} style={{ border: 'solid', borderColor: 'lightgray', borderWidth: '1px', padding: '0.5em' }}>
<span>Selection:</span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The space available for buttons is very limited ... might look better for more crowded screens (or smaller screens) to have these as small cards with Selection, Placement and Direction as title ... The buttons do spill over as soon as the menu is extended on the left which is the expected default UI.

@UlysseFG
Copy link
Collaborator

UlysseFG commented Jan 5, 2026

I really like the new look ... much cleaner.
I played around with it some and i am quite pleased in general ...
The only thing that should be change mechanically is that when you select a quadrant you can place as a quadrant (which should do nothing special) but the algorithm you implemented is so generic that it actually double down on "quadranting". It actually does what it is supposed to do but i suspect it might confuse the lab ... I am not sure how to treat that since you don't have a way to recognize the user intention but maybe we can reword the buttons/category to clarify. Maybe we can have Place: Sequentially | as Source | as Quadrant . Might be more obvious that they would want as source if they selected Quadrant 1.

@UlysseFG
Copy link
Collaborator

UlysseFG commented Jan 5, 2026

Maybe we should have :
Select : None | Invert | Quadrant , Place : Sequentially, as Source, as Quadrant, Place by : Column | Row ?

@UlysseFG
Copy link
Collaborator

UlysseFG commented Jan 5, 2026

Another thing that came to mind ... The tubes without parent should have the Selection Quadrant disabled and the Placement Quadrant Potentially activated... It is realistic to think maybe someone will want to place in quadrant samples that are unordered at first ... I see this would be an exception so maybe we can disable that too for now and ask Somayeh when we see her if this could be useful.

<Radio.Button key={PlacementType.SEQUENTIAL} value={PlacementType.SEQUENTIAL}>Sequentially</Radio.Button>
<Radio.Button key={PlacementType.SOURCE_PATTERN} value={PlacementType.SOURCE_PATTERN} disabled={activeSourceContainer.name === null}>as Source</Radio.Button>
<Radio.Button key={PlacementType.QUADRANT_PATTERN} value={PlacementType.QUADRANT_PATTERN} disabled={activeSourceContainer.name === null}>as Quadrant</Radio.Button>
<Radio.Button key={PlacementType.SEQUENTIAL} value={PlacementType.SEQUENTIAL}>Sequential</Radio.Button>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Sequence" here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

<Button onClick={transferAllSamples} disabled={!canTransferAllSamples} style={{ marginRight: '1em' }}>Place All Source</Button>
<Popconfirm
title={`Are you sure you want to undo selected samples? If there are no selected samples, it will undo all placements.`}
title={`Are you sure you want to undo
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reword it to put the question at the end ... "You are about to undo all placement. Do you want to proceed ?"
I would prefer for this warning to be specific to the situation ... if there is any selected placed samples, "You are about to undo X placements. Do you want to proceed ?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

<Button onClick={clearSelections}>None</Button>
<Button onClick={invertSelections}>Invert</Button>
<Dropdown menu={quadrantSelectionMenu} disabled={activeSourceContainer.name === null}>
<Button>Quandrant</Button>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quandrant

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DONE

return (state: RootState) => selector(selectPlacementState(state))
}

window.placement = (containerID: ParentContainerIdentifier | undefined) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place behind a debug mode test if you really need this in the code ...

// not considered serializable.
middleware: (getDefaultMiddleware) => getDefaultMiddleware({immutableCheck: false, serializableCheck: false}).concat(logger).concat(notificationError)
})
window.store = store // for debugging
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place behind a debug mode test if you really need this in the code ...

@UlysseFG
Copy link
Collaborator

UlysseFG commented Jan 7, 2026

Might be worth adding a space container around the destination buttons (even without any additional text) to align the tables on both sides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants