Prima dropdown sites page

[ukutaht]

Changes

Uses Prima Dropdown for cards on /sites page.

This took me down a deep rabbit-hole. The problem was that the pin/unpin animations would break LiveView rendering with Prima dropdowns nested in the cards.

I found that the issue is with phx-skip optimization applied by Liveview during diffing and rendering.

This optimization relies on either:
a) id attribute supplied by user in liveview code
b) phx-id attribute auto-generated by liveview if id is missing

Prima, in an effort to make its API more convenient, would auto-generate DOM IDs before wiring up the aria relationships. But since this happens in Javascript, it messes up Liveview diffing when the phx-skip optimization is applied. Liveview sees that an element has an id and assumes that it was added in Liveview Elixir code, meaning that it expects the patch coming from the server to have the same id. This was not the case with JS-generated IDs.

After lots of digging and experimenting my conclusion is that the only way to play nice with Liveview is to remove auto-generated IDs from Prima and to require the user to supply IDs for each DOM node. Unless Liveview changes its core mechanics, it is impossible to auto-generate IDs and keep Liveview rendering working in all cases.

As of 0.2.5 IDs will be required for all dropdown elements that need aria-attribute wiring (which is all of them). I need to make IDs required for modal and combobox elements as well, but that will come in the future.

The first commit changes all existing dropdowns to supply IDs, the second commit adds the new dropdown.

Dark mode

• The UI has been tested both in dark and light mode

[ukutaht]

Question for @sanne-san

The current dropdown in prod has an indigo highlight when site is pinned:

With this PR we would lose the highlighting because I'm using the basic dropdown icon class form here.

Do you think it's worth extending the dropdown icon class mechanism to also include something like 'highlighted' icons?

[zoldar]

@ukutaht sorry, maybe you have already looked into this - but have you tried modifying prima to use LiveView js commands to set IDs instead? I mean this.js().setAttribute(el, attr, val) from https://hexdocs.pm/phoenix_live_view/js-interop.html#js-commands. From what I see, prima is currently using DOM-native el.setAttribute. The LV command ensures (or at least should ensure) LiveView accounts for these changes when applying any future patches.

[ukutaht]

Thanks for that pointer @zoldar! I wasn't aware of this Liveview facility.

Unfortunately it doesn't work, I gave it a shot and the same bug is present. The phx-skip code seems to ignore the client-side attribute mapping.

But it does give me a good direction to look into. Perhaps there's some way to make Liveview respect the client-set id attribute during DOM patching. That would make sense and help build a nicer API for Prima.

I have a simple local liveview patch working although I suspect it's quite suboptimal. It does array lookups in presumably very performance-sensitive DOM patching operation. But it's good enough to demostrate the problem/solution space in general. I will see what the Liveview maintainers think about this, whether it's worth putting more effort into a better solution. It's possible they have no interest in allowing client-set DOM IDs for some design reasons I'm not currently aware of.

In any case, these discussions, extra work, merging and releasing will take time even if they agree that this is a problem worth fixing in the framework.

I'd prefer to get the current PR merged to keep things moving, and if in the future we can remove the hand-rolled IDs that will be a bonus. WDYT @zoldar ?

If you agree I'll fix the tests and make the PR mergeable.

[zoldar]

@ukutaht Of course, that's by no means a showstopper, just something that came to my mind when peeking at it.

[github-actions]

Preview environment👷🏼‍♀️🏗️
PR-6063

[zoldar]

LGTM on the basis of manual testing, though the automated tests need updating of course.

I still think it would be good to ensure DOM manipulation in hooks (and directly in templates via JS) is always done via LV JS commands as otherwise me might experience other weird issues during re-renders if/when interactions get more complex.

[ukutaht]

In any case, these discussions, extra work, merging and releasing will take time even if they agree that this is a problem worth fixing in the framework.

Hopefully I get some feedback here: phoenixframework/phoenix_live_view#4146

I still think it would be good to ensure DOM manipulation in hooks (and directly in templates via JS) is always done via LV JS commands as otherwise me might experience other weird issues during re-renders if/when interactions get more complex.

Completely agree. I wasn't aware of this liveview mechanism and I really appreciate it being there. I will work on converting all attribute setters in Prima to use it.