Fix table bugs #417
Fix table bugs #417
Conversation
alnasir7
commented
Feb 21, 2021
alnasir7
commented
- Switched function of arrows
- Default value of "none" for undefined fields
- bug fixes
- added sort icons
Codecov Report
@@ Coverage Diff @@
## master #417 +/- ##
=======================================
Coverage 72.40% 72.40%
=======================================
Files 30 30
Lines 5886 5886
=======================================
Hits 4262 4262
Misses 1624 1624
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
There was a problem hiding this comment.
- When the table is empty, you render nothing. You should still render the table column headers in this case, and have a td with full colspan that informs the user that the table is empty. This will avoid confusion on the user's part.
-
You are not able to search by all of the columns, just one, but you should be able to. For example, in the members table, you should be able to search by both the full name and the email address. You should show all entries that match either field.
-
The icons for the events table for ICS used to be colored. When improving a component, keep as many of the old (useful) features as you can.
- Using grid is suboptimal styling here, as the display becomes weird for certain screen sizes:
In the example above, you can see that there is more than enough space for both controls to be on the same line, but since you use grid the controls are forced to wrap. You should use floats instead.
-
Fix all other comments mentioned in this review.
-
Avoid using your own buttons when possible, and use Bulma buttons for style consistency. In this case, you can use
is-smalland something likeis-lightto replicate the button styles you have now.
alnasir7
force-pushed
the
fix-table
branch
2 times, most recently
from
6be2a86
to
f80636f
Compare
There was a problem hiding this comment.
You might not want to delete the icon files. The fact that they were there before you made this PR gives a strong indication that some icon is still using those files.
Otherwise looks good to me! Haven't executed the code though, will let you know if anything comes up. Merge after Armaan has completed the AWS migration.
I added the icons on the previous PR. They were not used before. In this PR I changed to something else so I removed them |
| <Icon name="chevron-down" /> | ||
| <div onClick={() => handleColumnsSort(column.Header)}> | ||
| {titleize(column.render('Header'))} | ||
| <span style={{ marginLeft: '1rem' }}> |
There was a problem hiding this comment.
It's hard to see which icons are being rendered in which conditions, so consider extracting this into a function that returns the icon name!
const iconNameForSortedState = (column): string => {
if(column.isSorted){
if(column.isSortedDesc) {
return "triangle-down"
}else{
return "triangle-up"
}
}
return "group-arrows"
}There was a problem hiding this comment.
This might still not be easy to do, in one of the cases I will be returning an empty string and not an icon.
| </button> | ||
| <button | ||
| style={{ marginRight: '0.5rem' }} | ||
| onClick={() => nextPage()} | ||
| className="is-light is-small" | ||
| onClick={() => gotoPage(pageCount - 1)} |
There was a problem hiding this comment.
I don't think. CanPrevious is automatically set to disabled when we are at page 1, so I don't see how that might happen.
|
@alnasir7 Do you mind walking me through this PR at some point? There seems to be a lot! "Table bugs" sounds pretty ominous too |
