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

[#1439] Add htmx to cases #621

Merged
merged 2 commits into from
Jun 14, 2023
Merged

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented May 16, 2023

No description provided.

@vaszig vaszig marked this pull request as draft May 16, 2023 14:35
@vaszig vaszig force-pushed the feature/1439-add-htmx-to-cases branch 2 times, most recently from e8a7f5a to 324ff43 Compare May 31, 2023 14:57
@vaszig vaszig force-pushed the feature/1439-add-htmx-to-cases branch 2 times, most recently from b1f6b03 to e3565c4 Compare June 9, 2023 06:47
@alextreme alextreme requested a review from Bartvaderkin June 12, 2023 18:37
@vaszig vaszig force-pushed the feature/1439-add-htmx-to-cases branch from b0aea11 to a27c45d Compare June 13, 2023 04:58
@vaszig vaszig marked this pull request as ready for review June 13, 2023 05:17
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2023

Codecov Report

Merging #621 (0d2388b) into develop (6be1f1c) will decrease coverage by 0.23%.
The diff coverage is 83.72%.

@@             Coverage Diff             @@
##           develop     #621      +/-   ##
===========================================
- Coverage    96.45%   96.23%   -0.23%     
===========================================
  Files          618      627       +9     
  Lines        21857    22282     +425     
===========================================
+ Hits         21083    21443     +360     
- Misses         774      839      +65     
Impacted Files Coverage Δ
src/open_inwoner/accounts/forms.py 97.76% <ø> (-0.20%) ⬇️
...inwoner/components/templatetags/pagination_tags.py 100.00% <ø> (ø)
src/open_inwoner/openzaak/notifications.py 95.12% <ø> (ø)
src/open_inwoner/utils/tests/playwright.py 78.72% <0.00%> (+0.82%) ⬆️
src/open_inwoner/cms/cases/tests/test_htmx.py 57.94% <57.94%> (ø)
src/open_inwoner/cms/cases/views/submissions.py 82.50% <82.50%> (ø)
src/open_inwoner/cms/cases/views/cases.py 89.39% <89.39%> (ø)
src/open_inwoner/cms/cases/views/mixins.py 96.84% <96.84%> (ø)
src/open_inwoner/cms/cases/forms.py 100.00% <100.00%> (ø)
src/open_inwoner/cms/cases/urls.py 100.00% <100.00%> (ø)
... and 7 more

... and 16 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vaszig
Copy link
Contributor Author

vaszig commented Jun 13, 2023

@jiromaykin FYI if you want to freeze the spinner and review it properly you can add a breakpoint to the cases (cms/cases/views/cases.py -> InnerOpenCaseListView.get_cases()) . Let me know if you need any help.

@vaszig vaszig requested a review from jiromaykin June 13, 2023 05:46

def handle_no_permission(self):
if self.request.user.is_authenticated:
return TemplateResponse(self.request, "pages/cases/403.html")
Copy link
Contributor

Choose a reason for hiding this comment

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

neat!

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

Htmx stuff looks pretty good.

One thing that bothers me is the javascript components don't chain from the node that is found by the selector, but use global selectors to grab the various sub-elements and attach event handlers.

This is not accurate and could give memory leaks because if the selector matches the new html fragment the component could still find and (duplicated) attach to elements outside of the fragment. Eg: multiple instantiations of the component code could connect to the same elements.

@jiromaykin
Copy link
Contributor

...FYI if you want to freeze the spinner and review it properly you can add a breakpoint to the cases[...].

Thanks! I am excited to test this!

Copy link
Contributor

@jiromaykin jiromaykin left a comment

Choose a reason for hiding this comment

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

Maybe it's just me, but when the list view has loaded (any list open/closed/Lopende) - I still see the spinner at the bottom after data has loaded (spinner should disappear here after loading is done),
When this happens it seems like javascript is becoming unresponsive because I cannot open the desktopmenu navigation anymore from this page.

listview-aanvraag

@jiromaykin
Copy link
Contributor

jiromaykin commented Jun 13, 2023

Maybe it's just me, but when the list view has loaded (any list open/closed/Lopende) - I still see the spinner at the bottom after data has loaded

Additionally: the main-navigation still works if I log in as a DigiD user - but it still has the spinner still visible after data has loaded.

@vaszig
Copy link
Contributor Author

vaszig commented Jun 13, 2023

Maybe it's just me, but when the list view has loaded (any list open/closed/Lopende) - I still see the spinner at the bottom after data has loaded

Additionally: the main-navigation still works if I log in as a DigiD user - but it still has the spinner still visible after data has loaded.

@jiromaykin have you reloaded-rebuilt js in your local machine?It seems weird because I don't seem to have a problem.

@jiromaykin
Copy link
Contributor

[...] have you reloaded-rebuilt js in your local machine?It seems weird because I don't seem to have a problem.

Sorry @vaszig - I thought the Watch function did a clean rebuild. It's working correctly after one build.

Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I still see a lot of global selectors (eg: ID's, getElementById(x) or querySelector("#x") in the components.

As everything works I'll approve to land this larger PR and added Taiga issue 1544 (and earlier 1511) to clean this up.

Comment on lines +11 to +17
const documentUpload = this.fileUploadInput.closest('#form_upload')

fileUploadInput.addEventListener('change', function (e) {
const files = e.target.files
const sizeInfo = documentUpload.querySelector('#upload_size')
const nameInfo = documentUpload.querySelector('#upload_name')
const validationInfo = documentUpload.querySelector('#upload_error')
const closeButton = documentUpload.querySelector('#close_upload')
const submit_upload = documentUpload.querySelector('#submit_upload')
Copy link
Contributor

@Bartvaderkin Bartvaderkin Jun 14, 2023

Choose a reason for hiding this comment

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

I discussed this with Vasilios and put it in ticket 1544

@alextreme alextreme merged commit da8da44 into develop Jun 14, 2023
@alextreme alextreme deleted the feature/1439-add-htmx-to-cases branch June 14, 2023 10:14
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.

5 participants