-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[RFC] New users table layout #1842
Conversation
I like search at the top for consistency in the rest of the app. Thanks for doing that. By any chance does anyone reading this thread have data on how admins use/want to use this page? It would be helpful to have to know what data we should display for them. A few thoughts:
A couple of things I thought of, but would probably need admin validation from a few different groups before we implemented as you noted the technical load would increase:
What do you think of those things? |
Stores are able to add as many roles as they like. We can't know the number of roles a store has. That's why checkboxes won't work IMO, but we could try to use a multi select field.
Yes
Absolutely
Would like to add this as a separate PR and then for all paginated tables, good idea 👍
Yes, this is already handled by #1844
These need extra information we don't have in all stores or are even tough to calculate. These are interesting filters, but belong more to an analytics or business intelligence tool IMO and are also very shop specific. I think we should only add the common information and filters on that table used by the majority of stores.
I already thought of those. We then need to make use of the new full-width layout for this table, what we should do anyway. Let me try those. Thanks for the feedback 🌹 |
d24108a
to
dfd9ab4
Compare
@Mandily @jhawthorn this is now ready to review. |
@tvdeyen Do you have a new screenshot you could share? I don't generally get into the code review part of reviewing :) |
Sure. Will add them later, as I am on mobile right now. Until then (and as a general tip): Consider to install With that you can easily pull down a PR and run the sandbox locally.
I always do this for reviewing layout related stuff. |
@Mandily the screenshot above is already updated. |
dfd9ab4
to
20cefcf
Compare
A few more things have come to mind since looking at this last:
Otherwise this looks good! |
Absolutely
These kind of queries are usually better inside a business intelligence tool. I already felt uncomfortable with adding these columns in the first place, but thought they provide some nice extra information on users (the same we display on the sidebar of the user detail screens). Actually I already tried implementing this, but it turns out that this is a quite complex query. Ransack (the tool we use for search, filtering and sorting) has no built in support for this. We would need to add complexity to the code base that I’m not sure of want to have. What do you think @jhawthorn @cbrundson? |
I discovered data tables a few weeks ago while looking into bootstrap sort functionality as a whole. Would this be helpful? https://datatables.net/examples/styling/bootstrap.html |
Data tables doesn’t help us, as we only load 10 results per page and data tables requires all data to be present at once. Even their ajax feature won’t help us as not the rendering is the problem, the searching and sorting of huge amounts of data (users x orders x line items) is a slow and complex query. And with that data tables can’t help us. I will try to implement something and then we can decide if we think it’s worth to keep in the core code base or if an extension would be something we should work on. |
dd719bf
to
655037c
Compare
Display users roles on the users table. Also adds a role filter to the users search form.
Adds a date range filter to the users search form and adds a sortable “Member Since” date column to the users table.
655037c
to
81e3235
Compare
The current admin users table looks like this
I think we can do better
But we can't know 100% what attributes a user has. Stores can define their own user classes. What all user classes have in common is defined in the
UserMethods
module that all stores need to include in their user class in order to work with Solidus.The commonalities are:
created_at
&updated_at
The user sidebar ("Lifetime Stats") displays following data:
created_at
timestampI think displaying all of these values in the table does not provide much value and the data is costly to gather. For a table of 20 users this could lead to lots of database queries.
So, I went with this approach
Please let me know what you think.
/c @Mandily @jhawthorn