- Notifications
You must be signed in to change notification settings - Fork928
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedApr 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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%" />} | |||
greyscaledApr 29, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: G r e y <grey@coder.com>
Uh oh!
There was an error while loading.Please reload this page.
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 unnestUserMenu and addLangauge for the hard-codedSuspend 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.