refactor (components): Unify <c-filter-drawer> with <c-dialog> - #7447
refactor (components): Unify <c-filter-drawer> with <c-dialog>#7447mexomagno wants to merge 14 commits into
Conversation
…eatures while maintaining contract
…n dialog on form errors
elisa-a-v
left a comment
There was a problem hiding this comment.
Nice, thank you! Refactor works and there are three things I liked that the issue didn't ask for:
variant="drawer"matches how<c-button>already handles variants and makes it easier for consumers to use the component without needing to know the exact classes required to make it work- the
motion-safe:prefixes give the transitions reduced-motion support the old drawer never had - the
DialogComponentTestcoverage, good call adding tests for this
I do have some comments that should be addressed before we merge this, left them all inline.
Side note: you can use GitHub's keywords to automatically link the issue to the PR so the issue will be closed once the PR gets merged. In this case, you can do "Closes #7318" instead of "Implements #7318" and that'll do it 👌🏽
| {% if has_errors %}data-has-errors{% endif %} | ||
| {{ attrs }} | ||
| x-on:open-filter-drawer="open" | ||
| :attrs="{% if has_errors %}{'data-has-errors': True}{% else %}{}{% endif %}" |
There was a problem hiding this comment.
I was actually surprised to see this worked, because attrs isn't a free prop name: Cotton fills it with whatever undeclared attributes the caller passed, and <c-dialog> renders it onto its root div. That's how x-on:open-filter-drawer on the line above gets there, so setting :attrs explicitly only works because Cotton merges them, which is non-obvious and isn't behavior I'd want to depend on.
I'm assuming the corner you were working around is that you can't put a conditional attribute on a <c-dialog> tag the way you could on the plain <div> this used to be, but I think there's a cleaner fix. Pass the prop like this:
| :attrs="{% if has_errors %}{'data-has-errors': True}{% else %}{}{% endif %}" | |
| data-has-errors="{{ has_errors|yesno:'true,false' }}" |
Then in docket_filter.js we can do:
// If the filter form was submitted with errors, pop the mobile
// drawer open so the user can see the validation messages inside.
- const drawer = this.$el.querySelector("[data-has-errors]");
+ const drawer = this.$el.querySelector('[data-has-errors="true"]');That way we keep the filter-specific prop out of the dialog component, and we don't touch Cotton's attrs, thus avoiding potentially weird edge cases.
| </c-library.item> | ||
| <c-library.item optional only> | ||
| <c-slot name="label"><code>disable_transitions</code></c-slot> | ||
| <c-slot name="description">Opt-out of dialog transitions on open and close. Only affects the <code>drawer</code> variant</c-slot> |
There was a problem hiding this comment.
"Only affects the drawer variant" isn't right. The overlay's {% if not disable_transitions %} guard sits outside the variant check, so the flag strips the overlay fade on the default centered dialog too, and in fact test_disable_transitions_removes_overlay_transitions renders the default variant and asserts exactly that.
Maybe we could do:
| <c-slot name="description">Opt-out of dialog transitions on open and close. Only affects the <code>drawer</code> variant</c-slot> | |
| <c-slot name="description">Opt out of dialog transitions on open and close. Disables the overlay fade on every variant, and the panel slide on the <code>drawer</code> variant.</c-slot> |
| {{ panel }} | ||
| </div> |
There was a problem hiding this comment.
Indentation slipped here:
| {{ panel }} | |
| </div> | |
| {{ panel }} | |
| </div> |
| </c-library.item> | ||
| </c-library.list> | ||
|
|
||
| <c-library.list title="Slots" only> |
There was a problem hiding this comment.
This one is optional and not something you introduced: the trigger_button slot has been undocumented since it was added, how about we add something like this:
| <c-library.list title="Slots" only> | |
| <c-library.list title="Slots" only> | |
| <c-library.item optional only> | |
| <c-slot name="label"><code>trigger_button</code></c-slot> | |
| <c-slot name="description">Replaces the default trigger button entirely. Use when the trigger needs its own styling or content, as <code>c-filter-drawer</code> does.</c-slot> | |
| </c-library.item> |
| <section class="max-w-full w-full border-t-2 border-greyscale-200" x-intersect.margin.-100px="show" id="filter-drawer"> | ||
| <h2 class="mt-6 mb-3">Filter drawer</h2> | ||
| <p>A mobile slide-in panel for filter controls. Only visible below the <code>md</code> breakpoint. Reuses the dialog composable for open/close, escape key, and auto-close on breakpoint change. Auto-opens when <code>data-has-errors</code> is present.</p> | ||
| <p>A mobile slide-in panel for filter controls. Only visible below the <code>md</code> breakpoint. Reuses the <code>c-dialog</code> component which provides open/close, escape key, and auto-close on breakpoint change. Auto-opens when <code>data-has-errors</code> is present.</p> |
There was a problem hiding this comment.
All of this predates your PR, but since you already tweaked it, I think we can make it even better.
This paragraph describes data-has-errors, which is the attribute the component sets internally rather than anything a caller passes. From the outside the API is the has_errors prop, so this is probably more useful:
| <p>A mobile slide-in panel for filter controls. Only visible below the <code>md</code> breakpoint. Reuses the <code>c-dialog</code> component which provides open/close, escape key, and auto-close on breakpoint change. Auto-opens when <code>data-has-errors</code> is present.</p> | |
| <p>A mobile slide-in panel for filter controls. Only visible below the <code>md</code> breakpoint. Reuses the <code>c-dialog</code> component which provides open/close, escape key, and auto-close on breakpoint change. Auto-opens when <code>has_errors</code> is truthy.</p> |
Separately and optionally: has_errors isn't in this section's Props list either. Same as the trigger_button note: it's a nice-to-have more than a blocker.
There was a problem hiding this comment.
Not yours originally: these section markers (<!-- Trigger -->, and the same for Dialog, Overlay, and Panel below) are HTML comments, so they ship to users in page source. Django comments {# ... #} don't. Would you mind fixing them while you're on this file? 🙏🏽
Fixes
Closes #7318
Summary
This PR reimplements
filter_drawercomponent as adialogwrapper instead of an independent component depending on dialog composable to implement behavior that thedialogcomponent already has. It makes the effort of preserving the existingfilter_drawercontract.variantprop todialogto provide the "drawer" variantdisable_transitionsprop todialogto opt-out of transitionsfilter_drawercomponent to use c-dialogv2_components.html)dialogpropsDeployment
This PR should:
skip-deploy(skips everything below)skip-web-deployskip-celery-deployskip-cronjob-deployskip-daemon-deployTo deploy just update the frontend components. Nothing else is affected.