New team setup UI - #6666
Conversation
|
| details.querySelector('summary').focus() | ||
| }, | ||
|
|
||
| // Wires up the WAI-ARIA listbox-button keyboard pattern for a role picker: |
There was a problem hiding this comment.
issue, non-blocking: These handlers don't match the dashboard's top bar popovers from headless-ui (https://headlessui.com/react/popover). I think ideally, we should be consistent with that. Popover's handlers do less than this and that's good. Less to go wrong.
There was a problem hiding this comment.
This consistency sounds nice but it's difficult to achieve and the scope expands far beyond this PR. There's a split between LiveViews and the dashboard, and I just logically followed the LiveView example, aiming for consistency with other Prima components (just pushed a PR that creates this component in Prima: plausible/prima#25).
There was a problem hiding this comment.
Is the split justified though? From perspective of the user, it's all Plausible.
I understand the reluctance to rewrite this minor piece of UI again, but this extra complexity (over a headlessui popover) isn't justified for this component, at least I don't think so.
Who are these keyboard handlers for and what's our confidence that they serve the purpose?
All custom keyboard handlers risk messing with people that can't use a mouse to navigate, or see what's happening on the screen.
To them, the browser's default Home button behavior may be the escape from out of a messed up cookie banner that's preventing them from completing some essential task on a government website. Close your eyes, turn on screen reading, and try to set up an appointment to renew your passport. Another challenge: set up Plausible.
I'm not an expert on accessibility, but I do know that it takes expertise to build an accessible component.
Anyways, this is just me advising to take care here and in the future with custom keyboard handlers. It's a non-blocking comment because I can't prove that it's not perfectly accessible.
| # validate_exclusion/3 only runs its check when the field actually changed | ||
| # relative to the struct's current value, which would let a freshly | ||
| # auto-created team (whose name already equals the reserved default) keep | ||
| # that name simply by resubmitting it unchanged. Check unconditionally. | ||
| if get_field(changeset, :name) == Plausible.Teams.default_name() do | ||
| add_error(changeset, :name, "is reserved") | ||
| else | ||
| changeset | ||
| end |
There was a problem hiding this comment.
suggestion, non-blocking: There's a force_change function in Ecto. We ended up using it for annotations for a similar situation where I originally had built a workaround.
2 participants
Changes
This PR builds a new UI for the team setup page that is completely independent from TeamManagement code (which continues to work exactly the same way in team settings). The objective is to make the initial team setup UI cleaner in order to add team creation as an optional onboarding step.
The new UI is a simple form, where:
team_member_limit. Hitting the plus icon to add rows while at limit will not add any more rows and will display a "not-allowed" cursorNote: The handrolled
member-rows.jshook is quite a lot of code, but as a follow-up, I'm planning to introduce aPrima.Listboxcomponent and use it here. Somember-rows.jsshould eventually get rid of the hand-crafted keyboard navigation, focusing and blurring the dropdown, etc. We'll also be able to useListboxin the TeamManagement LiveView to make the UX feel more snappy in high-latency scenarios.Tests
Changelog
Documentation
Dark mode