-
Notifications
You must be signed in to change notification settings - Fork 905
feat: Add user menu on users table #1222
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
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1222 +/- ##
==========================================
+ Coverage 65.58% 65.73% +0.14%
==========================================
Files 269 272 +3
Lines 17337 17791 +454
Branches 162 216 +54
==========================================
+ Hits 11371 11695 +324
- Misses 4774 4872 +98
- Partials 1192 1224 +32
Continue to review full report at Codecov.
|
@@ -16,6 +17,8 @@ export const TableHeaders: React.FC<TableHeadersProps> = ({ columns }) => { | |||
{c} | |||
</TableCell> | |||
))} | |||
{/* 1% is a hack to make the table cell width fits the content */} | |||
{hasMenu && <TableCell width="1%" />} |
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.
Aside: Added ticket for storybook here: #1224
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.
I don't think we need to have a storybook for each table component, I think one for "Table" is good enough since it will use all the others. What do you think?
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.
@BrunoQuaresma In general I agree, but I can think of a reason it might be useful to storybook subcomponents: if they take props that make them render differently, a story for the subcomponent will allow you to play with those props and make different stories for them, whereas it's a little harder to do that in a story for just the composite component.
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.
Hm... I think that is ok when the component can be used outside of the "main" component but in this case, the TableHeader should be used inside of the Table component and it also does not have states or multiple behaviors 🤔. But I'm ok with doing the storybook for this component, we can discuss it better in a FE Variety meeting.
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.
If we decide against it and close the ticket, no harm is done. Wasn't intending to add context or weight here, just noticed it and made a ticket and moved on.
Co-authored-by: G r e y <grey@coder.com>
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.
Approving with the recommendation to unnest UserMenu
and add Langauge
for the hard-coded Suspend
operation ✅
@@ -16,6 +17,8 @@ export const TableHeaders: React.FC<TableHeadersProps> = ({ columns }) => { | |||
{c} | |||
</TableCell> | |||
))} | |||
{/* 1% is a hack to make the table cell width fits the content */} | |||
{hasMenu && <TableCell width="1%" />} |
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.
If we decide against it and close the ticket, no harm is done. Wasn't intending to add context or weight here, just noticed it and made a ticket and moved on.
@vapurrmaid, not a problem! I assigned all of them to me already. |
No description provided.