Conversation
stevekinney
left a comment
stevekinney
left a comment
There was a problem hiding this comment.
Just a few minor tweaks.
stevekinney
left a comment
stevekinney
left a comment
There was a problem hiding this comment.
You'll definitely want to get a design review. There seems to be some issues with the borders around the tables as well as the alignment of the headings. I think you're not going to be able to use a table for this view.
| <td class=""> | ||
| <a sveltekit:noscroll {href}> | ||
| <td> | ||
| <a sveltekit:noscroll {href} class="hover"> |
There was a problem hiding this comment.
What does a hover class do if it's permanently on?
|
|
||
| tr { | ||
| @apply bg-gray-50; | ||
| @apply border-2 flex justify-between; |
There was a problem hiding this comment.
Do you notice almost a double border on the rows?
| @apply bg-blue-100; | ||
| } | ||
|
|
||
| tr:hover .hover { |
There was a problem hiding this comment.
I'm not totally sure what's going on here. Does this seem repetitive to you? If I combine this class and the one above it, then it seems to perform mostly the same. I'm not totally sure that this hover class is the way forward. Unless the name has some other meaning?
| @@ -78,7 +79,4 @@ | |||
| .active:hover { | |||
There was a problem hiding this comment.
Is this being used anymore?
| <thead> | ||
| <tr class="bg-gray-200"> | ||
| <th class="w-3/12 text-left ">Workflow Name</th> | ||
| <th class="w-3/12 text-left ">Name</th> |
There was a problem hiding this comment.
Did you notice that the headings don't quite look right?
| {#each workflows as workflow} | ||
| <WorkflowsSummaryRow {workflow} {timeFormat} /> | ||
| {/each} | ||
| <VirtualList items={workflows} let:item> |
There was a problem hiding this comment.
This is kind of what we talked about the other day. I'm not convinced we can use a <table> for this view anymore. It the VirtualList might add something between the table heading and the table body that breaks the natural layout. I'm pretty sure you're going to have to fall back to an another element.
stevekinney
left a comment
stevekinney
left a comment
There was a problem hiding this comment.
Nice! It's getting there. Make sure you schedule a design review for this morning and we'll finish this up today.
| @apply bg-gray-200 text-gray-500 text-xs h-6 m-0 p-3 uppercase table-fixed; | ||
| .workflow-table { | ||
| display: block; | ||
| height: calc(100vh - 25%); |
There was a problem hiding this comment.
This definitely has some weird implications on the layout. When we scroll down, there is a lot of extra space. We probably are going to want to get rid of this. I'm not going to block this PR over it though.
Now that we have no sidebar and we're about to remove the navigation on the other side, we can probably simplify the layout and remove this later.
| </thead> | ||
| <slot name="rows" /> | ||
| </table> | ||
| <section class="workflow-table border-collapse w-full rounded-lg"> |
There was a problem hiding this comment.
The table itself should probably have a border along the sides. Right now, it's missing a border along the bottom entirely. I suggest adding the following classes: border-2 border-gray-300 overflow-hidden
| {workflow.id} | ||
| </div> | ||
| <div class="links w-3/12 text-left"> | ||
| <h3> |
There was a problem hiding this comment.
It's not a blocker, but I wonder if we really need the <h3> tags? 🤷🏻
- lodash: ^4.17.21 -> ^4.18.1 (CVE-2026-4800, CVE-2026-2950, CVE-2025-13465) - svelte: 5.53.3 -> 5.55.1 (CVE-2026-27902, CVE-2026-27901) - @sveltejs/kit: 2.53.0 -> 2.55.0 (DoS via form deserialization) - storybook: ^8.6.11 -> ^8.6.18 (CVE-2026-27148) - tar-fs: >=2.1.2 -> ^3.1.2 (CVE-2025-59343) Resolves Dependabot alerts #233, #232, #159, #204, #203, #194, #193, #207, #192, #127.
#3269) - lodash: ^4.17.21 -> ^4.18.1 (CVE-2026-4800, CVE-2026-2950, CVE-2025-13465) - svelte: 5.53.3 -> 5.55.1 (CVE-2026-27902, CVE-2026-27901) - @sveltejs/kit: 2.53.0 -> 2.55.0 (DoS via form deserialization) - storybook: ^8.6.11 -> ^8.6.18 (CVE-2026-27148) - tar-fs: >=2.1.2 -> ^3.1.2 (CVE-2025-59343) Resolves Dependabot alerts #233, #232, #159, #204, #203, #194, #193, #207, #192, #127.
2 participants
What was changed
Matches figma (not pixel perfect) with screen scrolling