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

disable and hide csv import button when request in flight #142

Merged
merged 24 commits into from
May 13, 2024
Merged

Conversation

freddieptf
Copy link
Collaborator

@freddieptf freddieptf commented Apr 17, 2024

Fixes #136
Fixes #63

Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

good idea and good change

src/server.ts Outdated Show resolved Hide resolved
src/routes/add-place.ts Outdated Show resolved Hide resolved
src/liquid/place/bulk_create_form.html Outdated Show resolved Hide resolved
src/liquid/place/bulk_create_form.html Outdated Show resolved Hide resolved
@kennsippell
Copy link
Member

The performance tweaks in this PR are spot-on.

For generation times, measured the change to enable cache: true in LiquidJS and it brought down time-to-generate by 80% (2.5s down to 0.5s). For response size, it brought responses sizes down 97% (from 3.5MB to ~125kb).

Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

Sorry! I didn't send the code review... I left in pending state

src/liquid/place/list_lazy.html Show resolved Hide resolved
src/liquid/place/bulk_create_form.html Outdated Show resolved Hide resolved
src/routes/app.ts Show resolved Hide resolved
src/liquid/place/bulk_create_form.html Outdated Show resolved Hide resolved
src/server.ts Show resolved Hide resolved
Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

I didn't see this review was pending. Apologies for the slow turnaround.
I'm really excited for this change - we had somebody on the group this week who uploaded a CSV twice.

  • I just did a quick test and I think the buttons in the rows aren't working - try clicking "upload", "trash can", or "refresh" on any row in the table and nothing happens.
  • During CSV upload could we maybe disable the cancel button too? It's clickable but I'm not sure if it does anything.

Seeing this error in console

TypeError: Cannot read properties of null (reading 'firstChild')
    at He (htmx.min.js:1:11222)
    at De (htmx.min.js:1:12038)
    at Fe (htmx.min.js:1:12842)
    at x (htmx.min.js:1:42365)
    at Tr (htmx.min.js:1:44027)
    at b.onload (htmx.min.js:1:39225)

Code looks good!

@freddieptf
Copy link
Collaborator Author

freddieptf commented May 8, 2024

I just did a quick test and I think the buttons in the rows aren't working - try clicking "upload", "trash can", or "refresh" on any row in the table and nothing happens.

oh i missed that, i think it's a combination of how swaps are done on the table and bigskysoftware/htmx#2247. I've updated the libraries and added support for individual row updates.

Edit: Looking into the swap errors now

@freddieptf
Copy link
Collaborator Author

@kennsippell I've made some changes to the events api and the updates should appear "faster" now. This should fix #63 as well

Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

Excellent change!
Remember to update package version before merge.

}

private eventedPlaceStateChange = (subject: Place | Place[], state: PlaceUploadState) => {
if (!Array.isArray(subject)) {
subject = [subject];
}

if (subject.length > 1) {
Copy link
Member

Choose a reason for hiding this comment

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

would love your thoughts on how to make this work with the warning system... with the warning system, when you edit CHP A you can cause changes to CHP B. I suppose I will now have to monitor all changes on all rows and separately trigger each refresh?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

when you edit CHP A you can cause changes to CHP B. I suppose I will now have to monitor all changes on all rows and separately trigger each refresh

Triggering individual events is the most optimal. Refreshing the table is basically just refreshing the page at this point

@@ -64,8 +63,7 @@

<td>
{% if place.creationDetails.password %}
{% capture explanation %}Username: {{ place.creationDetails.username }}
Password: {{ place.creationDetails.password }}{% endcapture %}
{% capture explanation %}Username: {{ place.creationDetails.username }} Password: {{ place.creationDetails.password }}{% endcapture %}
Copy link
Member

@kennsippell kennsippell May 9, 2024

Choose a reason for hiding this comment

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

i like having it on multiple lines for when you copy/paste
is there a reason to change and put it on one line? do you like it better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Anything after the new line will be ignored when pushing the html via sse https://developer.mozilla.org/en-US/docs/Web/API/Server-sent_events/Using_server-sent_events#event_stream_format

src/liquid/components/place_item.html Outdated Show resolved Hide resolved
src/routes/events.ts Outdated Show resolved Hide resolved
src/routes/events.ts Outdated Show resolved Hide resolved
src/routes/app.ts Outdated Show resolved Hide resolved
hierarchy: Config.getHierarchyWithReplacement(place.type, 'desc'),
userRoleProperty: Config.getUserRoleConfig(place.type),
},
place: place,
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
place: place,
place,

Copy link
Member

@kennsippell kennsippell left a comment

Choose a reason for hiding this comment

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

I've got a problem when submitting invalid csv files. No error is displayed.

@freddieptf freddieptf merged commit becba25 into main May 13, 2024
1 check passed
@freddieptf freddieptf deleted the btns branch May 13, 2024 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants