DT-1248 - consistent link styles #1589

rossedfort · 2023-08-24T20:24:02Z

Description & motivation 💭

Use <Link /> component wherever possible and adjust styles to match new figma designs.

Also refactor button to use CVA

Screenshots (if applicable) 📸

Design Considerations 🎨

Testing 🧪

How was this tested 👻

Manual testing
E2E tests added
Unit tests added

Steps for others to test: 🚶🏽‍♂️🚶🏽‍♀️

Checklists

Draft Checklist

Merge Checklist

Issue(s) closed

Docs

Any docs updates needed?

vercel · 2023-08-24T20:24:06Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
holocene	⬜️ Ignored (Inspect)	Visit Preview		Aug 31, 2023 6:04pm

Alex-Tideman · 2023-08-29T20:29:24Z

I think this button is a little too tall now, makes wonky spacing in the event history table. Or just make it items-center

Alex-Tideman · 2023-08-29T20:29:58Z

But dang that button is 🔥

src/lib/components/banner/banner.svelte

src/lib/components/import/event-history-file-import.svelte

src/lib/components/schedule/schedule-form-view.svelte

src/lib/components/workflow/filter-search/filter-list.svelte

src/lib/holocene/combobox/combobox.svelte

src/lib/holocene/scroll-to-top.svelte

laurakwhit · 2023-08-29T22:26:17Z

src/lib/holocene/split-button.svelte

  export let disabled = false;
  export let position: 'left' | 'right' = 'left';
-  export let href = '';
  export let primaryActionDisabled = false;
 </script>

 <MenuContainer class={$$props.class}>


Wondering if we want to do something with z-index for the border on hover for these buttons

I'll ask Ash about this, I'm not sure what the expected behavior here is.

src/lib/holocene/button.svelte

laurakwhit · 2023-08-29T22:41:56Z

src/lib/components/event/event-summary-table.svelte

@@ -3,7 +3,7 @@

  import EventCategoryFilter from '$lib/components/event/event-category-filter.svelte';


Looks like the links in the expanded row could use centering

I think this is the same issue that Tideman commented on, should be fixed 👍

laurakwhit · 2023-08-29T22:44:20Z

src/lib/components/event/event-summary-table.svelte

@@ -36,15 +36,15 @@
    <th class="w-44"><EventCategoryFilter {compact} /></th>
    <th class="w-auto xl:w-80">
      <div class="flex w-full justify-end">
-        <button
-          class="relative flex w-28 items-center justify-end rounded sm:justify-between"
+        <Button


Do we want Button styling for any of the filters (e.g. Date & Time) in the header too?

Yeah I think we need to sync up with Ash on the expected behavior here. The first 2 column headers are dropdown menus, which don't change background on hover, but the last column is a primary button with no dropdown.

src/lib/holocene/button.svelte

src/lib/components/workflow/filter-search/boolean-filter.svelte

src/lib/holocene/modal.svelte

Co-authored-by: Laura Whitaker <[email protected]>

src/lib/holocene/table/table-row.svelte

stevekinney · 2023-08-31T16:57:28Z

colors.cjs

@@ -21,7 +21,7 @@ const colors = {
  },
  indigo: {
    50: '#eef2ff',
-    100: '#e0e7ff',
+    100: '#E0EAFF',


I am not going to block on this, but the inconsistency between capital and lower case makes me sad.

Fair, I just copied the values from Figma, and figured we will be revisiting this file once Design finalizes our color palette.

stevekinney · 2023-08-31T16:58:06Z

src/lib/components/banner/banner.svelte

@@ -24,9 +25,9 @@
    class={`relative block text-center leading-10 ${severity}`}
    {...$$restProps}
  >
-    <a href={link} target="_blank" rel="noreferrer">
+    <Link href={link} newTab>


Maybe external as the prop? I hate the camel case props that don't jive with the rest of HTML.

Sure I agree, although newTab was already the name of the prop in <Link /> so I didn't change it. Can make a follow up PR for that.

stevekinney · 2023-08-31T16:58:46Z

src/lib/components/search.svelte

    {id}
+    labelHidden
+    icon={icon ? 'search' : null}


This ternary feels weird.

icon is a boolean prop that controls whether the magnifying glass icon displays in the input. icon={icon && 'search'} won't do, because then false would be passed to icon on <Input /> which is not valid. Could change null to undefined or '' but other than that I'm not sure how to make this better.

stevekinney · 2023-08-31T16:59:13Z

src/lib/components/workflow/filter-search/boolean-filter.svelte

@@ -23,7 +23,7 @@
 </script>

 <MenuContainer>
-  <MenuButton id="boolean-filter" controls="boolean-filter-menu">
+  <MenuButton unroundLeft class="!border-l-0" id="boolean-filter" controls="boolean-filter-menu">


What's going on with the important?

By default our inputs, buttons, menu buttons, etc. have borders and border radius, however there are instances where they are combined with other elements, and we want to remove that border and border-radius. I don't like the pattern of overriding default component styles by adding a css class, I would rather it be controlled by a prop so as to maintain visual consistency. That's exactly why I went with this approach for <Button />, I just haven't done it for <MenuButton />.

I'm not sure if important is necessary here though, we may just be able to pass the regular border-l-0 tailwind class, @laurakwhit did you happen to test that out?

I think it would be better to use the group or peer classes in Tailwind.

For example, it looks like we know this is the first of its kind the group. Our styling could have affordances for this without us needed to specify it.

I'm not sure if important is necessary here though, we may just be able to pass the regular border-l-0 tailwind class, @laurakwhit did you happen to test that out?

I did not. I just pulled that from somewhere similar.. Maybe ConditonalMenu?

stevekinney · 2023-08-31T16:59:36Z

src/lib/components/workflow/filter-search/list-filter.svelte

-    variant="search"
-    unroundLeft
+    variant="secondary"
+    borderRadiusModifier="square-left"


This prop feels forced. Why can't we just pass a class?

I think a prop is better so that as much of the css that affects how a component looks is in one place, i.e. the component itself. IMO adding a class to a component should be a last resort and only happen in extreme one-off cases, which we should probably try to avoid.

To the core problem, why would we need to manually pass this prop or class?

Because some of our interfaces make use of core components, like dropdown menu, button, input, etc. but need to be tweaked slightly for display with other components. I've yet to come up with a better way to solve this apart from making whole new sets of components that have square and/or borderless left or right edges, but it's on my back burner for sure.

Screen.Recording.2023-08-31.at.12.05.48.PM.mov

Screen.Recording.2023-08-31.at.12.07.04.PM.mov

clean up all links

1f62e84

rossedfort requested a review from a team as a code owner August 24, 2023 20:24

rossedfort requested review from stevekinney and removed request for a team August 24, 2023 20:24

Merge branch 'main' into DT-1248-link-hover-styles

55a7b7e

vercel bot deployed to Preview August 24, 2023 20:31 View deployment

rossedfort added 4 commits August 24, 2023 15:49

fix cy

32b6c62

fix pw

d6aa517

fix pw

9e08bbc

snaps

fed04a4

vercel bot deployed to Preview August 24, 2023 22:04 View deployment

rossedfort added 4 commits August 24, 2023 17:12

Merge branch 'main' into DT-1248-link-hover-styles

e355a82

fix cy

261c913

Merge branch 'main' into DT-1248-link-hover-styles

85e65dc

button overhaul with cva

37ec0af

vercel bot deployed to Preview August 25, 2023 21:13 View deployment

update tests and fix orderable list item

554d3ed

vercel bot deployed to Preview August 25, 2023 21:36 View deployment

lint

062c11f

vercel bot deployed to Preview August 25, 2023 21:55 View deployment

rossedfort added 2 commits August 25, 2023 15:56

add lint script to lintstagedrc

f9147ba

Merge branch 'main' into DT-1248-link-hover-styles

b2c4653

vercel bot deployed to Preview August 28, 2023 15:24 View deployment

rossedfort added 5 commits August 28, 2023 09:29

fix link

d3f634d

snaps

0a10849

fix cy

84f14c8

fix pw

9fcb2b2

fix icon button and button focus state

cc0bd74

vercel bot deployed to Preview August 28, 2023 17:25 View deployment

rossedfort added 2 commits August 29, 2023 10:52

add active state to btn

2960727

snaps

3c42e57

vercel bot deployed to Preview August 29, 2023 17:01 View deployment

clean up combobox a lil

2afb742

laurakwhit reviewed Aug 29, 2023

View reviewed changes

src/lib/holocene/button.svelte Show resolved Hide resolved

rossedfort added 3 commits August 30, 2023 09:01

fix clipping and alignment on event summary rows

dcd4487

focus combobox input on trigger click

c464b4d

change icon-button hover

7b18446

vercel bot deployed to Preview August 30, 2023 18:59 View deployment

rossedfort added 2 commits August 30, 2023 13:07

add unroundLeft to boolean filter

a9670f4

clean up some stuff from CR

882496f

vercel bot deployed to Preview August 30, 2023 19:55 View deployment

rossedfort added 2 commits August 30, 2023 13:57

remove table-header button hover styles

da154de

Merge branch 'main' into DT-1248-link-hover-styles

f2bcc21

vercel bot deployed to Preview August 30, 2023 20:24 View deployment

Merge branch 'main' into DT-1248-link-hover-styles

ccec0bd

vercel bot deployed to Preview August 30, 2023 21:44 View deployment

Merge branch 'main' into DT-1248-link-hover-styles

4a656c6

vercel bot deployed to Preview August 31, 2023 15:07 View deployment

laurakwhit reviewed Aug 31, 2023

View reviewed changes

src/lib/components/workflow/filter-search/boolean-filter.svelte Outdated Show resolved Hide resolved

src/lib/holocene/modal.svelte Show resolved Hide resolved

Update src/lib/components/workflow/filter-search/boolean-filter.svelte

f1779c1

Co-authored-by: Laura Whitaker <[email protected]>

laurakwhit reviewed Aug 31, 2023

View reviewed changes

src/lib/holocene/table/table-row.svelte Show resolved Hide resolved

stevekinney reviewed Aug 31, 2023

View reviewed changes

fix truncation on event-summary rows

4aa74d7

laurakwhit approved these changes Aug 31, 2023

View reviewed changes

rossedfort merged commit dd4389f into main Aug 31, 2023
10 checks passed

rossedfort deleted the DT-1248-link-hover-styles branch August 31, 2023 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DT-1248 - consistent link styles #1589

DT-1248 - consistent link styles #1589

rossedfort commented Aug 24, 2023 •

edited

Loading

vercel bot commented Aug 24, 2023 •

edited

Loading

Alex-Tideman commented Aug 29, 2023 •

edited

Loading

Alex-Tideman commented Aug 29, 2023

laurakwhit Aug 29, 2023

rossedfort Aug 30, 2023

laurakwhit Aug 29, 2023

rossedfort Aug 30, 2023

laurakwhit Aug 29, 2023

rossedfort Aug 30, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

stevekinney Aug 31, 2023

stevekinney Aug 31, 2023

laurakwhit Aug 31, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

stevekinney Aug 31, 2023

rossedfort Aug 31, 2023

		@@ -3,7 +3,7 @@

		import EventCategoryFilter from '$lib/components/event/event-category-filter.svelte';

DT-1248 - consistent link styles #1589

DT-1248 - consistent link styles #1589

Conversation

rossedfort commented Aug 24, 2023 • edited Loading

Description & motivation 💭

Screenshots (if applicable) 📸

Design Considerations 🎨

Testing 🧪

How was this tested 👻

Steps for others to test: 🚶🏽‍♂️🚶🏽‍♀️

Checklists

Draft Checklist

Merge Checklist

Issue(s) closed

Docs

Any docs updates needed?

vercel bot commented Aug 24, 2023 • edited Loading

Alex-Tideman commented Aug 29, 2023 • edited Loading

Alex-Tideman commented Aug 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rossedfort commented Aug 24, 2023 •

edited

Loading

vercel bot commented Aug 24, 2023 •

edited

Loading

Alex-Tideman commented Aug 29, 2023 •

edited

Loading