-
Notifications
You must be signed in to change notification settings - Fork 67
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
chore(ui): Object viewer expand/collapse button simplification #3308
base: master
Are you sure you want to change the base?
Conversation
Preview this PR with FeatureBee: https://beta.wandb.ai/?betaVersion=a196e0cccd969a3b9d1a916442b7dc1d0956e283 |
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.
tooltip="View collapsed" | ||
/> | ||
<Title | ||
onClick={() => setMode(mode === 'hidden' ? 'collapsed' : 'hidden')}> |
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.
total nit but i've been thinking that 1 liner functions are more aesthetic than I previously thought, we might consider adding a local isModeHidden
fn like:
function isModeHidden(mode) {
return mode === 'hidden'
}
function isModeExpanded...
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.
and then obviously use it everywhere we test the mode state
For sure - I thought it was an acceptable level of moving the cheese since we use the title collapse elsewhere for the entire viewer, and the collapse/expand buttons being separate buttons when they're really a toggle was kind of an anti-pattern and was cluttering it up. Yeah I saw the height re-size as well but I thought it may be better for a separate PR cause I know there was some stuff around setting the explicit height and virtualization to take into account. |
Description
Screenshot