Skip to content
This repository was archived by the owner on Feb 15, 2022. It is now read-only.

Bind to HeadlessUI to implement Flyout menus #385

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tmattio
Copy link
Contributor

@tmattio tmattio commented May 27, 2021

This is a warmup PR for me to get familiar with the stack, sorry if this is not addressing an open issue 🙂

This PR replaces the implementation of the flyout menus to use HeadlessUI (for which bindings are added).

The benefits are:

  • Capturing keyboard events to close the menus
  • Capturing clicks outside of the menu to close it
  • Handling the state transition nicely UI-wise
  • The implementation is a bit simpler

I'm opening this early, but there are still some problems to solve before merging:

  • Get the color back for the icons
  • Handling clicks on a link to close the menu

PS: I'm also configuring TailwindCSS to use the JIT mode, hopefully, this is not too controversial!

@vercel
Copy link

vercel bot commented May 27, 2021

@tmattio is attempting to deploy a commit to the ocaml Team on Vercel.

To accomplish this, @tmattio needs to request access to the Team.

Afterwards, an owner of the Team is required to accept their membership request.

@tmattio
Copy link
Contributor Author

tmattio commented May 27, 2021

@avsm The message above is strange, I thought you added me to the team already?

@ghost
Copy link

ghost commented May 27, 2021

PS: I'm also configuring TailwindCSS to use the JIT mode, hopefully, this is not too controversial!

I held this back intentionally (ocaml/ocaml.org#355 (comment)) because the amount of bug fix releases for JIT in tailwind CSS still seem high.

@ghost ghost mentioned this pull request May 27, 2021
6 tasks
@jonludlam
Copy link
Member

The code certainly looks a lot nicer to me :-)

@jonludlam
Copy link
Member

I would imagine the colour on the icons would come back with fill-current stroke-current text-orangedark

@tmattio
Copy link
Contributor Author

tmattio commented May 28, 2021

I held this back intentionally (#355 (comment)) because the amount of bug fix releases for JIT in tailwind CSS still seem high.

Ah, missed that!
I didn't notice any breakage with jit enabled, but I'll revert this bit if you prefer 🙂

@ghost
Copy link

ghost commented May 28, 2021

I didn't notice any breakage with jit enabled, but I'll revert this bit if you prefer 🙂

You can keep it, just try to scan through all the existing pages in development (watch) and production (serve) build mode, to ensure that it works throughout the site.

Also, we should ensure that the proper env is being set so that jit watcher mode (https://tailwindcss.com/docs/just-in-time-mode) is only activated when running make watch.

@ghost ghost mentioned this pull request May 28, 2021
10 tasks
@tmattio
Copy link
Contributor Author

tmattio commented May 28, 2021

Also, we should ensure that the proper env is being set so that jit watcher mode (https://tailwindcss.com/docs/just-in-time-mode) is only activated when running make watch.

I think you can use JIT all the time, it should be as fast as the previous compilation mode even for the first compilation.

@ghost
Copy link

ghost commented May 28, 2021

I expect JIT to always be used. I was thinking of this statement in the docs: "By default, Tailwind will start a long-running watch process if NODE_ENV=development, and it will run in one-off mode if NODE_ENV=production." Hopefully, nextjs sets this appropriately, but I wanted to us to confirm everything works as it should.

@avsm
Copy link
Member

avsm commented Jun 8, 2021

This seems good to merge, as it improves the menus quite a bit. Anything blocking it?

@tmattio
Copy link
Contributor Author

tmattio commented Jun 8, 2021

@avsm it's not quite complete yet. I'm planning on getting back to this as soon as the import work is complete on ood's side.

Most likely, I'll split out this PR in two: complete HeadlessUI bindings, and general improvement on the navigation/flyout menus.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants