fix: header breaks on mobile screens #374

hashrock · 2023-07-14T15:03:13Z

This pull request introduces a hamburger menu on mobile.

Close #345

Implementation notes:

I had to use descendant selectors and adjacent selectors to implement a pure CSS hamburger menu. This required using twind-preset-ext in Twind v1.
Two newly added icons use heroicons.
Requires few line of inline JS to be accessible with keyboard.

before:

after:

iuioiua

Looking good! Just a few technical concerns.

iuioiua · 2023-07-14T23:40:10Z

components/Header.tsx

+function MenuItem(props: MenuItemProps) {
+  return (
+    <a
+      href={props.href}
+      class={cx(
+        props.active ? ACTIVE_LINK_STYLES : LINK_STYLES,
+        "relative text-gray-500 px-3 py-4 sm:py-2",
+        props.class,
+      )}
+      aria-label={props.ariaLabel}
+    >
+      {props.name}
+    </a>
+  );
+}


I think it'd be cleaner if we just wrote the menu HTML without iterating over an array of elements. Menu items can vary in requirements, making a flexible <MenuItem /> challenging to manage past a certain point. We can have a MENU_ITEM_STYLES constant for any common styles between these menu items. WDYT?

Hmm I wanted to use the practice of defining menus with JSON, which used in our repos (Example), but okay let me revert last refactoring commit. let's discuss this in another PR

iuioiua · 2023-07-14T23:41:44Z

components/Header.tsx

-import { Bell, CircleFilled } from "./Icons.tsx";
-import { getToggledStyles } from "@/utils/display.ts";
+import { Bars, Bell, CircleFilled, Cross } from "./Icons.tsx";
+import { cx } from "@twind/core";


Sidenote: this might be able to replace the new getToggledStyles().

Yes, we can use it. I was just writing code like this at first:

class={cx( getToggledStyles( LINK_STYLES, ACTIVE_LINK_STYLES, props.url.pathname === "/pricing", ), NAV_ITEM, )}

I noticed that this code can be rewritten as:

class={cx( props.url.pathname === "/pricing" ? ACTIVE_LINK_STYLES : LINK_STYLES, NAV_ITEM, )}

This cx style description is used in deno deploy and can handle both concatenation and toggle of CSS strings.
Since this header component uses cx throughout, I removed getToggledStyles as a dependency.

I think it would be better to write getToggledStyles with a ternary operator where cx is used, what do you think?

What about if we got rid of getToggledStyles() completely across the codebase and used cx() instead? Maybe we could do this in another PR.

yeah if it's okay, I'll like to help to do this

iuioiua · 2023-07-14T23:43:24Z

components/Header.tsx

+      href={props.href}
+      class={cx(
+        props.active ? ACTIVE_LINK_STYLES : LINK_STYLES,
+        "relative text-gray-500 px-3 py-4 sm:py-2",


Suggested change

"relative text-gray-500 px-3 py-4 sm:py-2",

"relative text-gray-500 py-4 sm:py-2",

Perhaps, we don't need any horizontal padding. WYDT?

Hmm, maybe removing the padding makes the spacing a little too tight.

I think padding is necessary in order to widen the clickable area of the target.

with no padding:

with padding:

components/Header.tsx

brunocorrea23 · 2023-07-15T16:30:31Z

Looks great!
Suggestion: as it is such an important element of the UI, would it be better to leave the submit button outside the hamburger menu (ie: always visible)?

iuioiua · 2023-07-16T07:05:15Z

components/Header.tsx

+      href: "/account/notifications",
+      ariaLabel: "Notifications",
+    },
+    { name: "Submit a project", href: "/submit", class: "sm:hidden" },


Suggested change

{ name: "Submit a project", href: "/submit", class: "sm:hidden" },

{ name: "Submit", href: "/submit", class: "sm:hidden" },

I'm happy to keep the inner text the same.

Isn't it a little vague what to do if the label is just "Submit"? In PH, the label is only Submit, but the target is selected by hover.

Rather, what if the header button to say "Submit a project" when it is wide enough?

iuioiua · 2023-07-16T07:07:07Z

Looks great! Suggestion: as it is such an important element of the UI, would it be better to leave the submit button outside the hamburger menu (ie: always visible)?

I'd prefer to keep it in the menu as it is now. Instead, perhaps we could keep its button styling to stand out. WDYT?

Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>

hashrock · 2023-07-17T11:16:06Z

@iuioiua OK, wdyt?

iuioiua

This looks amazing 😍

iuioiua · 2023-07-17T11:23:36Z

I'll merge. Let's have the cx() discussion elsewhere, it needed.

hashrock · 2023-07-17T11:24:11Z

Thank you!

@hashrock

"Someone" broke the hamburger menu in #646. This fixes the issue and cleans things up by making the CSS a bit more direct: * I use https://tailwindcss.com/docs/hover-focus-and-other-states#styling-based-on-sibling-state to modify the `nav` directly. * I add IDs to the hamburger and X components, and then use https://tailwindcss.com/docs/hover-focus-and-other-states#using-arbitrary-variants to directly target them, in combination with peer. FYI @hashrock since this restores the great work you did in #374.

hashrock added 10 commits July 14, 2023 17:01

wip

1025b48

Merge remote-tracking branch 'origin/main' into fix-header-mobile

6dba21f

fix: mobile nav layout

56d1179

use twind/core/cx

b8946ed

fix

5c8409a

use SITE_BAR_STYLES

0371541

reorg twind classes

13d6452

refactor

88cb4a5

fix active style regression

6c7b26a

refactor

c1dccf3

hashrock requested a review from iuioiua July 14, 2023 15:03

fix types

9be9372

iuioiua suggested changes Jul 14, 2023

View reviewed changes

iuioiua reviewed Jul 16, 2023

View reviewed changes

Update components/Header.tsx

85fb0d5

Co-authored-by: Asher Gomez <ashersaupingomez@gmail.com>

deno-deploy bot deployed to Preview July 17, 2023 07:42 View deployment

hashrock added 2 commits July 17, 2023 20:10

revert MenuItem component

88f65c8

keep submit button style

ab29ed1

iuioiua approved these changes Jul 17, 2023

View reviewed changes

iuioiua merged commit 1c0ccd5 into main Jul 17, 2023

iuioiua deleted the fix-header-mobile branch July 17, 2023 11:24

deer mentioned this pull request Feb 21, 2024

fix: hamburger menu styling on smaller screens #671

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: header breaks on mobile screens #374

fix: header breaks on mobile screens #374

hashrock commented Jul 14, 2023

iuioiua left a comment

iuioiua Jul 14, 2023

hashrock Jul 17, 2023

iuioiua Jul 14, 2023

hashrock Jul 17, 2023 •

edited

Loading

iuioiua Jul 17, 2023 •

edited

Loading

hashrock Jul 17, 2023

iuioiua Jul 14, 2023

hashrock Jul 17, 2023

brunocorrea23 commented Jul 15, 2023

iuioiua Jul 16, 2023

hashrock Jul 17, 2023

iuioiua commented Jul 16, 2023

hashrock commented Jul 17, 2023

iuioiua left a comment

iuioiua commented Jul 17, 2023

hashrock commented Jul 17, 2023

	"relative text-gray-500 px-3 py-4 sm:py-2",
	"relative text-gray-500 py-4 sm:py-2",

	{ name: "Submit a project", href: "/submit", class: "sm:hidden" },
	{ name: "Submit", href: "/submit", class: "sm:hidden" },

fix: header breaks on mobile screens #374

fix: header breaks on mobile screens #374

Conversation

hashrock commented Jul 14, 2023

iuioiua left a 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

hashrock Jul 17, 2023 • edited Loading

Choose a reason for hiding this comment

iuioiua Jul 17, 2023 • edited Loading

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

brunocorrea23 commented Jul 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuioiua commented Jul 16, 2023

hashrock commented Jul 17, 2023

iuioiua left a comment

Choose a reason for hiding this comment

iuioiua commented Jul 17, 2023

hashrock commented Jul 17, 2023

hashrock Jul 17, 2023 •

edited

Loading

iuioiua Jul 17, 2023 •

edited

Loading