Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Style cleanup on tables and <ActionsDropdown /> #63

Merged
merged 5 commits into from
Apr 24, 2024
Merged

Style cleanup on tables and <ActionsDropdown /> #63

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
87d0086
src/lib-components: styling cleanup on <DynamicTable /> and <ActionsD…
DavidLarsKetch Apr 22, 2024
326039d
Update src/lib-components/lists/DynamicTable.vue
DavidLarsKetch Apr 23, 2024
dfb5af9
src/lib-components/lists/DynamicTable: removes tbody divide line
DavidLarsKetch Apr 23, 2024
7c72b60
src/lib-components/lists/DynamicTable: widens sort click target by in…
DavidLarsKetch Apr 23, 2024
6ebadf6
src/lib-components/lists/DataTable: adds no border, striped rows
DavidLarsKetch Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 51 additions & 48 deletions src/lib-components/lists/DynamicTable.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,54 +244,56 @@ loadAndRender()
class="px-6 py-3 text-xs font-medium tracking-wider text-gray-900 uppercase bg-gray-50 leading-4"
:class="col.alignment"
>
<span v-if="col.title">{{ col.title }}</span>
<span
v-if="col.sort"
class="cursor-pointer"
@click.prevent="handleSort(col.sort as string)"
>
<svg
v-if="currentSort !== col.sort"
xmlns="http://www.w3.org/2000/svg"
fill="none"
viewBox="0 0 24 24"
stroke="currentColor"
class="h-5 inline"
>
<path
stroke-linecap="round"
stroke-linejoin="round"
stroke-width="2"
d="M8 9l4-4 4 4m0 6l-4 4-4-4"
/>
</svg>
<svg
v-else-if="currentSortDirection == 'desc'"
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 20 20"
fill="currentColor"
class="h-5 inline"
<div class="flex items-center gap-2">
DavidLarsKetch marked this conversation as resolved.
Show resolved Hide resolved
<span v-if="col.title">{{ col.title }}</span>
<span
v-if="col.sort"
class="cursor-pointer"
@click.prevent="handleSort(col.sort as string)"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did find the target a bit small, personally, so I'll take a look.

>
<path
fill-rule="evenodd"
d="M14.707 12.707a1 1 0 01-1.414 0L10 9.414l-3.293 3.293a1 1 0 01-1.414-1.414l4-4a1 1 0 011.414 0l4 4a1 1 0 010 1.414z"
clip-rule="evenodd"
/>
</svg>
<svg
v-else
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 20 20"
fill="currentColor"
class="h-5 inline"
>
<path
fill-rule="evenodd"
d="M5.293 7.293a1 1 0 011.414 0L10 10.586l3.293-3.293a1 1 0 111.414 1.414l-4 4a1 1 0 01-1.414 0l-4-4a1 1 0 010-1.414z"
clip-rule="evenodd"
/>
</svg>
</span>
<svg
v-if="currentSort !== col.sort"
xmlns="http://www.w3.org/2000/svg"
fill="none"
viewBox="0 0 24 24"
stroke="currentColor"
class="h-5 inline"
>
<path
stroke-linecap="round"
stroke-linejoin="round"
stroke-width="2"
d="M8 9l4-4 4 4m0 6l-4 4-4-4"
/>
</svg>
<svg
v-else-if="currentSortDirection == 'desc'"
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 20 20"
fill="currentColor"
class="h-5 inline"
>
<path
fill-rule="evenodd"
d="M14.707 12.707a1 1 0 01-1.414 0L10 9.414l-3.293 3.293a1 1 0 01-1.414-1.414l4-4a1 1 0 011.414 0l4 4a1 1 0 010 1.414z"
clip-rule="evenodd"
/>
</svg>
<svg
v-else
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 20 20"
fill="currentColor"
class="h-5 inline"
>
<path
fill-rule="evenodd"
d="M5.293 7.293a1 1 0 011.414 0L10 10.586l3.293-3.293a1 1 0 111.414 1.414l-4 4a1 1 0 01-1.414 0l-4-4a1 1 0 010-1.414z"
clip-rule="evenodd"
/>
</svg>
</span>
</div>
</th>

<!--Table Actions Header-->
Expand All @@ -306,6 +308,7 @@ loadAndRender()
<tr
v-for="(row, rowIdx) in rows"
:key="rowIdx"
class="even:bg-gray-50"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@profsmallpine profsmallpine Apr 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we feel about this option?

Striped, no border between rows, header to bg-gray-100 from bg-gray-50.
Screenshot 2024-04-23 at 11 02 49 AM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@click="$emit('click:row', row.rowData)"
>
<template v-for="(cell, cellIdx) in row.cells" :key="cellIdx">
Expand Down Expand Up @@ -341,7 +344,7 @@ loadAndRender()

<tr v-if="!hasContent">
<td
:colspan="rows.length"
:colspan="columns.length + (hasActions ? 1 : 0)"
class="px-6 py-4 text-sm text-gray-700 whitespace-nowrap leading-5"
>
No items were found!
Expand Down
2 changes: 1 addition & 1 deletion src/lib-components/navigation/ActionsDropdown.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

:disabled="!hasActions"
>
<span class="sr-only">Open options</span>
Expand Down
Loading