Skip to content

Conversation

@david-crespo
Copy link
Collaborator

@david-crespo david-crespo commented May 7, 2024

  • Bump dep
  • Fix DocsPopover
  • Fix Listbox mostly
  • Fix broken e2e tests
  • Make sure z-index is still good

https://tailwindcss.com/blog/headless-ui-v2

Cool changes, but it seems like they're bundling in Floating UI in addition to ours, so our bundle got a few % bigger. I think we can eliminate our direct use of it though.

@vercel
Copy link

vercel bot commented May 7, 2024

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

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview May 21, 2024 7:46pm

className="DocsPopoverPanel z-10 w-96 rounded-lg border bg-raise border-secondary elevation-1"
ref={refs.setFloating}
style={floatingStyles}
anchor={{ to: 'bottom end', gap: 12 }}
Copy link
Collaborator Author

@david-crespo david-crespo May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How great is this! Need to make sure autoUpdate still works. Update: it seems to work. The thing moves with the page on scroll.


.ox-menu {
@apply max-h-[17.5rem] overflow-y-auto rounded border bg-raise border-secondary elevation-2;
@apply !max-h-[17.5rem] overflow-y-auto rounded border bg-raise border-secondary elevation-2;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! just to make this stronger than some height the component is applying. But maybe there's a better way.

@david-crespo
Copy link
Collaborator Author

Managed to fix the instance create e2e, but didn't finish z-index. I think they are both caused by the same change in headlessui: if you look at the options for Listbox 2.0, there is a modal option that defaults to true:

modal Boolean
Whether to enable accessibility features like scroll locking, focus trapping, and making other elements inert.

I suspect the listbox in headlessui 1.7 did not do this. We can get that behavior back with modal=false (it does make the z-index test pass!), but I bet leaving it on and accommodating our tests to that is better for accessibility. So it's worth doing a little research into what modal mode does and whether we can still do that (somewhat silly) z-index test with it on.

@charliepark
Copy link
Contributor

Still looking into z-index, but the specific failing assertion is await expectObscured(page.getByRole('option', { name: 'All projects' }))

@david-crespo
Copy link
Collaborator Author

Looks like the modal prop does exactly what it says in the docs: scroll lock and set others inert.

https://github.com/tailwindlabs/headlessui/blob/300e9eb95777210fd43570d31aae7da5b87d381f/packages/%40headlessui-react/src/components/listbox/listbox.tsx#L928-L938

@david-crespo
Copy link
Collaborator Author

david-crespo commented May 20, 2024

If scroll lock works, the test might not make sense anymore, because it’s about what happens when you scroll while the listbox is open. Maybe we can find another way to test that the z indexes are set up right.

@david-crespo
Copy link
Collaborator Author

Ok, well the scroll lock part does not work (which explains why the test is able to scroll the page). I suspect this is because the container we are scrolling is not the whole document but rather the content pane. We've talked about changing this in the past because it's slightly annoying for stuff like this, but it's a slightly invasive change, going from a grid where one pane scrolls to a classic thing where the frame has fixed position and floats on top of the scrolling document and you use padding to make sure everything lines up.

@david-crespo
Copy link
Collaborator Author

After some clicking around, I think listbox looks good but I notice the docs popover has different behavior from main on a tiny window. Not too important but I do like the behavior on main better. Probably something to tweak in the anchor prop.

image

@charliepark
Copy link
Contributor

I believe the issue with that floating modal is the "flip" middleware, which "will swap the placement of the panel if there is not enough room".

https://github.com/tailwindlabs/headlessui/blob/main/packages/%40headlessui-react/src/internal/floating.tsx#L234-L237

Trying to see what options we have to just always show it in the designated anchor position.

@david-crespo
Copy link
Collaborator Author

Good find. I think if it's much of a pain to change we can just live with it. You really can't put too much pressure on a 200px high window to look right.

@charliepark
Copy link
Contributor

Yeah, I think it might be asking a lot of a window that small.

@david-crespo
Copy link
Collaborator Author

Let’s party.

@david-crespo david-crespo merged commit 39b4491 into main May 22, 2024
@david-crespo david-crespo deleted the headless-2 branch May 22, 2024 00:02
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b) bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e) oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e) oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca) oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9) oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63) oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745) oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9) oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb) oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f) oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946) add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb) oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd) oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7) upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d) oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459) oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78) oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7) oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5) oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99) oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9) oxidecomputer/console#2223
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request May 22, 2024
Highlights: soft image validation, logout button on error pages to help
deal with auth-related errors.

oxidecomputer/console@078d171...a228b75

* [a228b75b](oxidecomputer/console@a228b75b)
bump omicron (only one tiny diff in validators)
* [fc91ec1e](oxidecomputer/console@fc91ec1e)
oxidecomputer/console#2256
* [39b4491e](oxidecomputer/console@39b4491e)
oxidecomputer/console#2230
* [e4e912ca](oxidecomputer/console@e4e912ca)
oxidecomputer/console#2247
* [dcf09ec9](oxidecomputer/console@dcf09ec9)
oxidecomputer/console#2217
* [c36b3d63](oxidecomputer/console@c36b3d63)
oxidecomputer/console#2238
* [a8eb7745](oxidecomputer/console@a8eb7745)
oxidecomputer/console#2251
* [9b20b7c9](oxidecomputer/console@9b20b7c9)
oxidecomputer/console#2248
* [f20a5bcb](oxidecomputer/console@f20a5bcb)
oxidecomputer/console#2245
* [b815dd8f](oxidecomputer/console@b815dd8f)
oxidecomputer/console#2244
* [8c7b2946](oxidecomputer/console@8c7b2946)
add node_modules to eslint ignore patterns
* [90e78dbb](oxidecomputer/console@90e78dbb)
oxidecomputer/console#2237
* [b603d2dd](oxidecomputer/console@b603d2dd)
oxidecomputer/console#2242
* [bfce37c7](oxidecomputer/console@bfce37c7)
upgrade @oxide/openapi-gen-ts to 0.2.2
* [efceb17d](oxidecomputer/console@efceb17d)
oxidecomputer/console#2236
* [1aa46459](oxidecomputer/console@1aa46459)
oxidecomputer/console#2235
* [b400ae78](oxidecomputer/console@b400ae78)
oxidecomputer/console#2225
* [7bb3bbf7](oxidecomputer/console@7bb3bbf7)
oxidecomputer/console#2229
* [c56a9ec5](oxidecomputer/console@c56a9ec5)
oxidecomputer/console#2228
* [cd9d1f99](oxidecomputer/console@cd9d1f99)
oxidecomputer/console#2227
* [ee269bd9](oxidecomputer/console@ee269bd9)
oxidecomputer/console#2223
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants