-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: 6867 6868 image and dot opacity #1029
Conversation
imageUnderlay: RootState["controls"]["imageUnderlay"]; | ||
// eslint-disable-next-line react/no-unused-prop-types -- used in shouldShowOpenseadragon | ||
layoutChoice: RootState["layoutChoice"]; | ||
// eslint-disable-next-line react/no-unused-prop-types -- used in shouldShowOpenseadragon | ||
config: RootState["config"]; | ||
// eslint-disable-next-line react/no-unused-prop-types -- used in shouldShowOpenseadragon | ||
panelEmbedding: RootState["panelEmbedding"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the image toggle button to Embedding
component, so we no longer need these props here
imageOpacity: number; | ||
dotOpacity: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new redux state for both opacities
client/src/components/embedding/components/Opacities/components/Opacity/index.tsx
Show resolved
Hide resolved
import { RootState } from "../../../../reducers"; | ||
import { Props, StateProps } from "./types"; | ||
|
||
function Opacities({ imageOpacity, dotOpacity, dispatch }: Props) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opacities
component hosts both types of opacity control
// eslint-disable-next-line react/no-unused-prop-types -- used in shouldShowOpenseadragon | ||
config: RootState["config"]; | ||
// eslint-disable-next-line react/no-unused-prop-types -- used in shouldShowOpenseadragon | ||
panelEmbedding: RootState["panelEmbedding"]; | ||
imageUnderlay: RootState["controls"]["imageUnderlay"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Embedding component now needs these props to determine whether to show the Image toggle button and its dropdown or not
@@ -146,7 +158,7 @@ const Embedding = (props: Props) => { | |||
</div> | |||
} | |||
> | |||
<Tooltip | |||
<Tooltip2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upgrade to Tooltip2 as a bonus
<Button | ||
type="button" | ||
data-testid="toggle-image-underlay" | ||
icon="media" | ||
intent={imageUnderlay ? "primary" : "none"} | ||
active={imageUnderlay} | ||
onClick={() => { | ||
track( | ||
/** | ||
* (thuang): If `imageUnderlay` is currently `true`, then | ||
* we're about to deselect it thus firing the deselection event. | ||
*/ | ||
imageUnderlay | ||
? EVENTS.EXPLORER_IMAGE_DESELECT | ||
: EVENTS.EXPLORER_IMAGE_SELECT | ||
); | ||
dispatch({ | ||
type: "toggle image underlay", | ||
toggle: !imageUnderlay, | ||
}); | ||
}} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing code moved here
<Popover2 | ||
onOpening={handleLayoutChoiceClick} | ||
position={Position.BOTTOM_LEFT} | ||
content={<Opacities />} | ||
> | ||
<ImageDropdownButton | ||
type="button" | ||
data-testid="image-underlay-dropdown" | ||
icon="caret-down" | ||
/> | ||
</Popover2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the new pop over that shows Opacities control as its content
@@ -1115,7 +1125,7 @@ class Graph extends React.Component<GraphProps, GraphState> { | |||
return createColorQuery(colorMode, colorAccessor, schema, genesets); | |||
} | |||
|
|||
updateOpenSeadragon() { | |||
initOpenSeadragon() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update the name to reflect its actual functionality
49d7440
to
15bf212
Compare
e5a7ece
to
d17f372
Compare
26658b5
to
af96c05
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, Timmy! I just see a couple things that would be great to fix.
- The "Image Opacity" and "Dot Opacity" labels should use the same style as the "Standard Categories" label.
- Clicking outside of the opacity popover (and the embedding popover actually) to dismiss it doesn't work as expected. It only works if you click outside of the main center workspace (i.e., one of the sidebars or the inline with the toolbar)
This isn't from this PR but it'd also be nice to fix the position of the cell count. It should be left aligned with the embedding selector icon. It appears to be shifted over because of the color legend, but it looks weird when the user isn't coloring by a continuous variable. Ideally, the color legend would appear below the cell count.
Ohh eagle eye 🦅 👁️!! Thanks so much, Harley! Will address all 3 👍 Also I need to investigate the failing e2e tests HMM! |
376260d
to
02d29bd
Compare
@hthomas-czi okay all 3 issues should be address now 😄 PTAL again and thanks again for catching them! ![]() ![]() Screen.Recording.2024-07-11.at.8.12.20.AM.mov |
@@ -132,6 +132,7 @@ const Header = (props: HeaderProps) => { | |||
<Right> | |||
<LinkWrapper> | |||
<Popover2 | |||
hasBackdrop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding backdrop creates a reliable way to detect click outside of the popover element
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks, Timmy.
Thanks so much, Harley! Looks like I need to update the e2e tests. Will do that after my meetings! |
3c7ae9e
to
452693e
Compare
452693e
to
01db94a
Compare
Okay finally all passed lol Merging! |
Opacities
component to control both image and dots opacityEmbedding
componentKNOWN ISSUES
Screenshots:
Video:
Screen.Recording.2024-07-09.at.10.12.03.AM-1.mov