-
Notifications
You must be signed in to change notification settings - Fork 0
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
OrthoMCL - Custom tree-table for OrthoGroup page #904
Conversation
… the Mesa inline option
…cally fixed-height mode)
…de (basically fixed-height mode)" This reverts commit f215762.
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.
This is looking really good. I made some comments below. Let me know what you think!
<div | ||
style={{ | ||
flexGrow: 1, | ||
width: 1 /* arbitrary non-zero width seems necessary for flex */, | ||
}} | ||
> | ||
<Global | ||
styles={globalStyle` | ||
.DataTable { | ||
margin-bottom: 0px !important; | ||
width: 80%; | ||
} | ||
`} | ||
/> | ||
<Mesa state={tableState} /> | ||
</> | ||
</div> |
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.
Sorry, I'm just noticing this. I'm not sure we should use Global
here, as the style will be reflected on all subsequent pages.
Instead, could you use a css
prop on the parent div
component (line 96), and target .DataTable
in there?
<div
css={{
flexGrow: 1,
width: 1 /* arbitrary non-zero width seems necessary for flex */,
'.DataTable': {
marginBottom: '0px !important',
width: '80%'
}
}}
>
<Mesa state={tableState} />
</div>
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.
Oh yeah, that's much simpler (and no side effects) - done in 5c896a0 - thanks!
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.
Could we change these identifiers such that they include the "group" prefix? E.g., GroupTreeResponse
, groupTreeResponseDecoder
, etc?
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.
Sure! fd446f8
…ogroup-tree-table__1254-fixes
…ogroup-tree-table__1254-fixes
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.
Just one possible oversight - see the comment.
Works great - so much better than before!
className={cancelBtnClassName} | ||
style={ | ||
cancelBtnRightMargin ? { right: cancelBtnRightMargin } : undefined | ||
}, [delayMs, searchTerm, onSearchTermChange]); |
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.
Not using delayMs
?
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.
Not using
delayMs
?
We should fix and test this before merging, @dmfalke
…boxList/SelectList generics 1 of 2
…ixes Orthogroup tree table 1254 fixes
I am planning to merge this PR. We can make a new issue for any remaining tasks we intend to implement. |
Testing:
There are only a few tree files available on the back end, so only a few orthogroups work. Here's are the valid OGs at the moment:
Run
ortho-site
locally and access the path/app/record/group/OG6_102443
Back end config for
.env
isTo do:
From the #1140 branch
UX suggestions