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

[Admin] Create new Tax Categories #5674

Merged

Conversation

spaghetticode
Copy link
Member

@spaghetticode spaghetticode commented Feb 28, 2024

This PR migrates the creation of new tax categories to the new admin interface.

The new category form is rendered via a modal dialog, which is added to the categories list page via turbo frame. Successful creation leads to a turbo stream page refresh, which updates the category list preserving the query params and the scroll position, for a consistent UX. To enable all the latest turbo features, turbo-rails has been updated to version 2.

The PR revisits the way our modal component works to make it act like a standard modal. We probably want to revisit existing usage of the modal component to make it compliant with the new behavior and take advantage of Turbo features following the path shown in this PR.

Additionally, the PR solved an issue with filtering on the tax categories list page, which was not functioning correctly.

Please look at the attached video for a visual demo of the fixes and the new feature.

Screen.Recording.2024-03-05.at.09.55.50.mov

Checklist

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • [I have used clear, explanatory commit messages]-
  • I have added automated tests to cover my changes.
  • I have attached screenshots to demo visual changes.

@spaghetticode spaghetticode requested a review from a team as a code owner February 28, 2024 15:05
@github-actions github-actions bot added changelog:solidus_core Changes to the solidus_core gem changelog:solidus_admin labels Feb 28, 2024
@spaghetticode spaghetticode force-pushed the spaghetticode/admin-new-tax-category branch from a1f978a to 8f8519f Compare February 28, 2024 15:06
@kennyadsl
Copy link
Member

As discussed during the core team meeting, I'd give turbo frame a go for this modal, which I have the feeling can be simplified.

@spaghetticode spaghetticode force-pushed the spaghetticode/admin-new-tax-category branch 2 times, most recently from 82097d5 to d863b25 Compare March 5, 2024 09:11
@spaghetticode
Copy link
Member Author

@kennyadsl I've just updated the PR and its description, video included. It's now leveraging Turbo frames and Turbo streams refreshes.

Spec failures are related to the latest release of the money gem (tags have not been pushed yet, so I'm not sure what change broke it), which seems to replace the EURO currency symbol from HTML entity (€) to unicode char ().
I'll open a PR with the fix later.

Screenshot 2024-03-05 at 10 37 59

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

❤️ wonderful execution, seems straightforward. Thanks, Andrea!

@spaghetticode
Copy link
Member Author

I tracked down the money gem issue to this PR. The change is included in 6.18.0, but not in 6.17.0, both released today 🤷 .

@spaghetticode spaghetticode force-pushed the spaghetticode/admin-new-tax-category branch 2 times, most recently from e266d89 to efc79a7 Compare March 7, 2024 08:49
We need to whitelist the ransackable attributes used on the new admin
tax categories page, or filtering won't work.
The former ones were not correct, probably just a leftover from
a cut and paste.
The component now behaves like a standard dialog:

  * closes on the current page, rather than redirecting elsewhere;
  * the close button is a standard dialog closing form;
  * the `open` attribute is removed, thus allowing ESC keypress
    to close the modal, and creating a focus trap when open. The
    previous behavior (i.e. have the modal open by default) is
    achieved via the new Stimulus controller, which on connect
    opens the dialog via JS.
This way we can inject arbitrary turbo frames into pages rendered
via this component.
The correct method name is `page_actions`, for this reason it was
previously not showing. Also, href is changed to a new route from
the new admin.
The new version adds cool new features such as page morphing and
refreshes. Specifically, we're going to use turbo stream refreshes
in a later commit.
The `new` action will render a modal dialog above a list of tax
categories, like the `index` action. For this reason, the common
controller code is extracted to a private method.
The component renders:

1) a list of tax categories in the background;
2) a dialog modal with the form for creating a new resource.

The modal is wrapped into a turbo frame tag, so it can be enclosed
in other pages (i.e. the tax categories index) asynchronously.
The `#create` controller action allows the creation of a new tax
category. Failed creation re-renders the form with errors within
the turbo frame tag, while successful creation is handled via a
turbo stream refresh tag that reloads the page.

Scroll position is preserved thanks to the custom Turbo meta tag
that enables the feature globally on the new admin.

Integration specs are added to test the complete feature.
@spaghetticode spaghetticode force-pushed the spaghetticode/admin-new-tax-category branch from efc79a7 to 5584f93 Compare March 7, 2024 09:37
Copy link
Contributor

@rainerdema rainerdema left a comment

Choose a reason for hiding this comment

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

Amazing work! 🌟


export default class extends Controller {
connect() {
this.element.showModal();
Copy link
Contributor

Choose a reason for hiding this comment

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

; semicolons can be omitted.

@spaghetticode spaghetticode merged commit 9c03da2 into solidusio:main Mar 11, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants