Style cleanup on tables and <ActionsDropdown /> #63
Conversation
DavidLarsKetch
requested review from
seedifferently,
steven-klein and
profsmallpine
| <span | ||
| v-if="col.sort" | ||
| class="cursor-pointer" | ||
| @click.prevent="handleSort(col.sort as string)" |
There was a problem hiding this comment.
Moving the click event and cursor-pointer up to the wrapping div will give users a larger clickable target that includes the text and icon. A bit out of scope since we haven't had that up to this point, so I'll leave that to you to decide. @click.prevent="col.sort ? handleSort(col.sort as string) : undefined" should conditionally apply click handling.
There was a problem hiding this comment.
I did find the target a bit small, personally, so I'll take a look.
| @@ -29,7 +29,7 @@ const { floatingStyles } = useFloating(trigger, wrapper, { | |||
| <Menu as="div" class="relative flex justify-end items-center"> | |||
| <MenuButton | |||
| ref="trigger" | |||
| class="w-8 h-8 bg-white inline-flex items-center justify-center text-gray-700 rounded-full hover:text-gray-900 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 disabled:opacity-50 disabled:cursor-not-allowed" | |||
| class="w-8 h-8 inline-flex items-center justify-center text-gray-700 rounded-full hover:text-gray-900 focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 disabled:opacity-50 disabled:cursor-not-allowed" | |||
There was a problem hiding this comment.
Just a note: this should be OK. The only other places I recall using ActionsDropdown is in DescriptionLists and on the mastermind group show page. Letting the parent background through, is probably the most desirable style in most use cases.
| @@ -306,6 +308,7 @@ loadAndRender() | |||
| <tr | |||
| v-for="(row, rowIdx) in rows" | |||
| :key="rowIdx" | |||
| class="even:bg-gray-50" | |||
There was a problem hiding this comment.
I'm good with making this our default, though I subjectively think we could drop the border between rows when they are striped. The extra border adds to the visual competition. Archive uses tables quite a bit more than Portal, so @profsmallpine should probably weight in on moving all in on striped tables.
I'd expect we'd move the DataTable to the same style as well for consistency.
There was a problem hiding this comment.
I don't find one as more compelling than the other, but I think it is best to pick one, either using border or stripes. The downside of stripes for me is that it makes even rows the same background color as the header which I find visually distracting to have the same emphasis for different things. Typically, the headers will loose their background shading so that you put the visual emphasis on the differentiation of rows.
There was a problem hiding this comment.
How do we feel about this option?
Striped, no border between rows, header to bg-gray-100 from bg-gray-50.
There was a problem hiding this comment.
Yep, should be OK. We use bg-gray-100 as the app shell background color, so outside a card wrapper it might be a touch off, but not to a degree I think we need to sweat right now. I think Portal currently wraps all tables in some kind of card like wrapper.
Co-authored-by: Steven Klein <[email protected]>
…cluding title; darkens header background
DavidLarsKetch
changed the title
On
<DynamicTable />...flexcontainerbg-gray-50on even rows to help visually differentiatebg-whitefrom the<ActionsDropdown />menu icon since that may or may not mesh with the row's background colorBefore
After
Screen.Recording.2024-04-22.at.12.00.10.PM.mov