Skip to content

Replace Dashboard's Database Table with an HTMX Grid #612

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 10 commits into from
Closed

Replace Dashboard's Database Table with an HTMX Grid #612

wants to merge 10 commits into from

Conversation

agavgavi
Copy link
Contributor

As the title states, this replaced the Vue mtable used to updated and display databases with instead the built in Grid, and it is still a SPA through HTMX.

This has almost all of the same features as the other table, besides filtering/searching. It seems like HTMX and search seem to have a slight miscommunication and doesn't properly pass the query parameters to the url which stops it from being properly searchable. In addition, the searching functionality in the grid isn't as generic as it is in mtable, which leads to an issue where the only known field in any table is the id, so that would be the only thing that could be searched.

This can be adjusted if a good way is found.

This PR is left as a draft such that others can test and comment on the direction I have gone.

Thanks
~Andrew

@jpsteil
Copy link
Collaborator

jpsteil commented Jul 31, 2021 •
edited
Loading

I pulled it down and am testing with my SouthBreeze database. All seems good so far. Will continue testing. Excellent work!

@jpsteil
Copy link
Collaborator

jpsteil commented Jul 31, 2021

I don't know how the previous dbadmin or web2py dbadmin worked, but it seems to me it would make sense to limit the grid display to only 4 or 5 fields so they all fit on the page nicely.

I'd also prefer to see bulma or bootstrap formatting but that is just my preference. And it adds dependencies to the framework, so probably not a plausible idea.

@agavgavi
Copy link
Contributor Author

Personally I'd agree with changing the style to something more standard than this black on blue, but with the predefined classes that isn't that hard to change later.
I'll change it to 5 entries per page when I return home later!

@jpsteil
Copy link
Collaborator

jpsteil commented Jul 31, 2021

Thanks Andrew

Just to be clear, I was referring to 5 columns wide, not entries (rows) per page.

What do others think? Is 5 the right number or is there some better way to determine how many columns to display?

As for layout, as much as I'd prefer to use bulma, it probably makes most sense to use no.css. Thoughts from others?

@agavgavi
Copy link
Contributor Author

agavgavi commented Aug 3, 2021

The 5 columns is easy and makes sense, I'll push a commit with that if it's what people want. As for no.css it makes sense to do that but that would require an entire redesign of the rest of the dashboard for uniformity which should be a larger discussion and probably a different PR.

@jpsteil
Copy link
Collaborator

jpsteil commented Aug 3, 2021

I'd be in favor of merging if some others can test as well. I tested with a few of my databases, but I tend to design them all similarly. So, may be good if someone else could give it a go too.

@agavgavi agavgavi marked this pull request as ready for review August 3, 2021 06:34
@agavgavi
Copy link
Contributor Author

agavgavi commented Aug 3, 2021

5 columns definitely looks better, I've restricted it to 6 to account for the ID column, and also updated a formatter in the action because there is an issue with the boolean formatter. The check if value else "" works for others but with booleans if the value is false it will not show anything since it will fail that check. Checking to see if it was an instance of boolean works better.

I'll mark this PR ready for Review which will allow it to be merged after others look over it some more.

The only thing I'm not sure on is if there is a good way to reintroduce searching/filtering in a generic enough way that allows for the tables to be filterable and how to get that working with HTMX. This can be discovered in a latter PR if necessary but for now this has all functionality a table would probably need.

@agavgavi agavgavi changed the title Replace Dashboard's Database Table with an HTMX Grid - Request For Comment Replace Dashboard's Database Table with an HTMX Grid Aug 3, 2021
@KellerKev
Copy link
Collaborator

Just got around to finally testing this. It looks great, but it seems that I can not edit the password field in the auth_user table?

@KellerKev
Copy link
Collaborator

KellerKev commented Aug 6, 2021 •
edited
Loading

I have the same behaviour in my grid for the auth_user table, so it has nothing to do with the column restrictions, to 5-6 columns which I removed anyway during testing. I think form does not process password fields atm. even though it should. The widget code is there in forms.py

@agavgavi
Copy link
Contributor Author

agavgavi commented Aug 6, 2021

This is because through the definition of the auth table in auth.py, the password field is marked readable=False, writable=False.

It's changed in the register form through DefaultAuthForm to writable=True.

I'm not sure if there is a good way to go about doing that here without a specific condition for the auth table which removes its general abstraction. I could loop through all fields in all tables and mark them writable but that would have bad side effects most likely and not the right design.

@mdipierro
Copy link
Contributor

works well. But before we merge 1) there still stylistic issues and 2) there is no search feature. We cannot merge it if removes functionality.

@agavgavi
Copy link
Contributor Author

agavgavi commented Aug 8, 2021

I'll work on the search functionality tomorrow, I'm not sure the best way to go about searching in a generic enough pattern that could account for all table setups but I'll spend the day investigating.

As for styling, if there are specifics you'd like changed let me know.

@agavgavi
Copy link
Contributor Author

I've pushed a commit that re-includes search for all readable fields. It uses a generic search function that can be made more specific for different types (for example we'd want to do .contains on strings versus == for integers). I'm not sure if pydal already has a generic way of searching that could be used instead but this seems to be working.

Kevin brought up a good point in discord that we may want all fields to be writable and readable since this is an "admin" grid. That is something that should be open to discussion and I'm down for either way, it's up to others more experienced than myself whether we want that or not.

@agavgavi
Copy link
Contributor Author

agavgavi commented Sep 4, 2021

I think this can be merged, if there is something that it's missing/blocking it from being merged let me know and I'll update it.

@mdipierro
Copy link
Contributor

mdipierro commented Sep 4, 2021 via email

@KellerKev
Copy link
Collaborator

KellerKev commented Sep 4, 2021 via email

@agavgavi
Copy link
Contributor Author

agavgavi commented Sep 4, 2021

@Kkeller83 That has been changed and works now. As well as updated styling for the detail/create/edit forms

@KellerKev
Copy link
Collaborator

nice, will test again tomorrow. Should be fine then .Thanks!

mdipierro
mdipierro reviewed Sep 5, 2021
View reviewed changes
apps/_dashboard/__init__.py
query = lambda value: field.contains(value)
return query

search_queries = [[f"By {field.label}", genericSearch(field)] for field in table]
Copy link
Contributor

Choose a reason for hiding this comment

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

this for now will do but we need something better

@mdipierro
Copy link
Contributor

I just tried it. It is really sleek. I am committing to merge this in the next 2 days but I want to improve the search because it is not sufficient for my use cases. Well done!

@mdipierro
Copy link
Contributor

Made a change to search. Please help me test it.

There is still a problem. If there is an error server side (invalid query, 404) there is not error reported to the user and the search appear working.

@mdipierro
Copy link
Contributor

On a second look I do not understand what HTMX buys us in this example. I removed it from your code. How about this? #631

@agavgavi
Copy link
Contributor Author

agavgavi commented Sep 6, 2021

Yes it work without HTMX just fine the reason I went for it was to keep the Single-Page App aspect of the old Mtable in the new dashboard to feel snappy. If it's preferable to have it just be the normal html version that's fine but I like the responsiveness of the HTMX.

@mdipierro
Copy link
Contributor

mdipierro commented Sep 6, 2021 via email

@agavgavi
Copy link
Contributor Author

agavgavi commented Sep 6, 2021

Fine by me I agree that the URL parameters functionality of HTMX is a nuisance so if this is the best solution I am all for it.

@agavgavi
Copy link
Contributor Author

agavgavi commented Oct 6, 2021

Should I close this so the other one can be merged?

@agavgavi agavgavi closed this Jan 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdipierro mdipierro mdipierro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agavgavi @jpsteil @KellerKev @mdipierro