SUM-45 | @rebeccahongsf | Theme & style header #3
Conversation
| <span className={clsx("bg-black-true block transition-all duration-300 ease-out h-[3px] w-full rounded-sm", {"-rotate-45 -translate-y-4": menuOpen, "translate-y-0.5": !menuOpen})}/> | ||
| <span | ||
| className={clsx( | ||
| "bg-black-true block transition-all duration-300 ease-out h-[3px] w-full rounded-sm", |
There was a problem hiding this comment.
Is there a reason we use h-[3px] instead of just h-3 here? Also, I see that we have a mix of rem and px units (eg, line 72, line 225 etc - I'm totally fine with border width or border radius be px but with width/height/font sizes, perhaps it's better we go with rem? 👀 I understand there might be exceptions.
| @@ -43,7 +43,7 @@ export const Lockup = ({ | |||
|
|
|||
| if (!suLockupEnabled) { | |||
| return ( | |||
| <div className="py-10"> | |||
| <div> | |||
There was a problem hiding this comment.
we probably don't need the div here. Less markup is often a good way to go. But if we do need it, totally fine. I've seen some videos that suggest to remove elements if there's no attributes, or use fragments: <>
| }: Props) => { | ||
| return ( | ||
| <> | ||
| {sumSiteHeaderPrim && <Button href={sumSiteHeaderPrim.url} secondary>{sumSiteHeaderPrim.title}</Button>} |
There was a problem hiding this comment.
Shouldn't the secondary button have the secondary styles?
There was a problem hiding this comment.
I would think so too but the figma mocks display it like so
There was a problem hiding this comment.
We can just switch them around, make the "Primary" from Drupal be the right button. "Secondary" be the left. So it'll still work with the designs, just swapped order basically.
There was a problem hiding this comment.
My concern is around the readability and assumption our content authors will make (e.g. assuming the buttons will appear left to right, header primary then secondary button regardless of styles).
If we are going to swap the orders, should we include help text around the styling of the button link? Or will the headings (e.g. Header Primary Button) be enough to inform the content authors? 😄
There was a problem hiding this comment.
That's ok, we can add some help text. It'll be better to keep the "Primary" link with the "Primary" styles IMO.
… main menu; remove unnecessary div in lockup; fixup formatting; remove unnecessary group-focus; add mobile chevron underline
src/components/menu/main-menu.tsx
Outdated
|
|
||
| const menuLevelsToShow = 2; | ||
|
|
||
| type Props = { | ||
| type Props = | ||
| Omit<StanfordBasicSiteSetting, "__typename" | "id" | "metatag"> & { |
There was a problem hiding this comment.
Just define and pass the 2 fields instead of the whole thing. Especially since this is a "use client" component, we want to only pass the most minimal data needed to work.
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
READY FOR REVIEW
Summary
Review By (Date)
Review Tasks
Setup tasks and/or behavior to test
Site Configuration Sync
Front End Validation
Associated Issues and/or People