Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTMX CSQL Fixture UI #35372

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

HTMX CSQL Fixture UI #35372

wants to merge 5 commits into from

Conversation

esoergel
Copy link
Contributor

Product Description

Technical Summary

Feature Flag

Safety Assurance

Safety story

Automated test coverage

QA Plan

Migrations

  • The migrations in this code can be safely applied first independently of the code

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@esoergel esoergel mentioned this pull request Nov 14, 2024
4 tasks
@AddisonDunn
Copy link
Contributor

Looks like a great approach, very smooth and simple.

Comment on lines 4 to 9
<form hx-post="{{ request.path_info }}"
hq-hx-action="save_expression"
hx-swap="outerHTML"
x-data="{ isUnsaved: false }"
@change="isUnsaved = true;"
@submit="isUnsaved = false;">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<form hx-post="{{ request.path_info }}"
hq-hx-action="save_expression"
hx-swap="outerHTML"
x-data="{ isUnsaved: false }"
@change="isUnsaved = true;"
@submit="isUnsaved = false;">
<form
hx-post="{{ request.path_info }}"
hq-hx-action="save_expression"
hx-swap="outerHTML"
x-data="{ isUnsaved: false }"
@change="isUnsaved = true;"
@submit="isUnsaved = false;"
>

{{ form.csql }}
</div>
<div class="col-2">
<button type="submit" class="btn btn-outline-primary" x-bind:disabled="!isUnsaved">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<button type="submit" class="btn btn-outline-primary" x-bind:disabled="!isUnsaved">
<button
class="btn btn-outline-primary" type="submit"
:disabled="!isUnsaved"
>
  1. I like to always have class be the first attribute
  2. if there is a HTMX or alpine attribute (:bind in this case), we want it on a second line
  3. I prefer shorthand alpine bindings, but could be convinced otherwise?

Comment on lines 24 to 28
<button class="btn btn-outline-danger"
hx-post="{{ request.path_info }}"
hq-hx-action="delete_expression"
hx-swap="delete"
hx-target="closest form">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<button class="btn btn-outline-danger"
hx-post="{{ request.path_info }}"
hq-hx-action="delete_expression"
hx-swap="delete"
hx-target="closest form">
<button
class="btn btn-outline-danger"
hx-post="{{ request.path_info }}"
hq-hx-action="delete_expression"
hx-swap="delete"
hx-target="closest form"
>

Comment on lines 25 to 30
<button type="button"
class="btn btn-default"
hx-post="{{ request.path_info }}"
hq-hx-action="new_expression"
hx-target="#csql-fixture-rows"
hx-swap="beforeend">
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<button type="button"
class="btn btn-default"
hx-post="{{ request.path_info }}"
hq-hx-action="new_expression"
hx-target="#csql-fixture-rows"
hx-swap="beforeend">
<button
class="btn btn-default" type="button"
hx-post="{{ request.path_info }}"
hq-hx-action="new_expression"
hx-target="#csql-fixture-rows"
hx-swap="beforeend"
>

Comment on lines +21 to +23
{% for form in csql_fixture_forms %}
{{ form }}
{% endfor %}
Copy link
Member

Choose a reason for hiding this comment

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

yes, this was exactly the approach i was thinking when reviewing the page with @AddisonDunn. Less front end logic being front-loaded from the backend into Alpine, and more on the python side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was super pleased to see how well this plays with Django templates! I'm still not totally sold on the second commit here though - I do think I liked it better before adding Django forms, but if I find the time I might try a compromise - like maybe I make a partial template with the <form> tags and the buttons, but have the fields themselves rendered by the Django form (at present, I set a custom template on the form, so it contains everything, which sounds nice in principle, but means I lose error handling in the template)

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