v3.ocaml.org-rescript icon indicating copy to clipboard operation
v3.ocaml.org-rescript copied to clipboard

Bind to HeadlessUI to implement Flyout menus

Open tmattio opened this issue 4 years ago • 11 comments

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!

tmattio avatar May 27 '21 15:05 tmattio

@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.

vercel[bot] avatar May 27 '21 15:05 vercel[bot]

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

tmattio avatar May 27 '21 15:05 tmattio

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

I held this back intentionally (https://github.com/ocaml/v3.ocaml.org/issues/355#issuecomment-842747264) because the amount of bug fix releases for JIT in tailwind CSS still seem high.

ghost avatar May 27 '21 15:05 ghost

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

jonludlam avatar May 28 '21 00:05 jonludlam

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

jonludlam avatar May 28 '21 00:05 jonludlam

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 🙂

tmattio avatar May 28 '21 07:05 tmattio

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 avatar May 28 '21 14:05 ghost

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.

tmattio avatar May 28 '21 15:05 tmattio

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.

ghost avatar May 28 '21 15:05 ghost

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

avsm avatar Jun 08 '21 14:06 avsm

@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.

tmattio avatar Jun 08 '21 14:06 tmattio