-
Couldn't load subscription status.
- Fork 819
Prototyping User Table Design Updates #13808
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
Prototyping User Table Design Updates #13808
Conversation
Build Artifacts
|
3b76d19 to
df13990
Compare
67a40dc to
35548a5
Compare
| </KGrid> | ||
|
|
||
| <div class="flex-column"> | ||
| <div class="flex-column table-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole component may no longer be necessary as the business logic has been extracted to just be a kind of widget in PaginationActions.vue that can be slapped anywhere with the given props and will handle the pagination buttons/messaging w/out the opinionated layout enforced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like with the new changes this component is no longer necessary yes, or at least it should be renamed in the scope of this PR so we can avoid future confusions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it feels like it would be better to just defer most of this to the consuming component, until we have a strong use case to generalize again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make a follow-up issue to clean this up a bit as it'll involve updating other areas as well and this PR is growing grey hairs
bb6e723 to
082c044
Compare
- Created useUsersTableSearch composable for search/query handling - Created usePagination composable for pagination state management - Refactored UsersTableToolbar to be a pure layout component - Updated UsersRootPage to use new composables - Integrated PaginationActions component for consistent pagination UI - Cleaned up UsersTable component to remove duplicated logic This refactoring improves maintainability by: - Encapsulating search and pagination logic in reusable composables - Following Vue 2 Composition API patterns used in the codebase - Creating clear separation of concerns between components - Providing flexible two-row layout structure for toolbar actions Initial Claude Prompt: 1) Get a sense of the files I've been working on based on my last git commit and current unstaged changes. 2) Review those changes to get a sense of how the code is structured. 3) The kolibri/plugins/facility/assets/src/views/users/common/UsersTableToolbar.vue and the packages/kolibri-common/components/PaginatedListContainer.vue components have some logic too tightly couple to their scope and aren't used in a way that makes it easy to, for example, know what page we're on. Also, the "query/search" handling in the overall changes I've made reflects to me something that would be easier to use if it were encapsulated somehow, such as into its own composable module. (NOTE: look at files use*.js in the codebase for an example of how we use the Composition API modules in Vue 2). In any case - the changes I'm working on are to rearrange some of the actions that we've put onto the screen. The #topRow and #bottomRow slots in packages/kolibri-common/components/PaginatedListContainerWithBackend.vue demonstrate at least the overall layout I want - basically I need two rows in a flex container (column dir) which each can then have their own flexbox divs w/ their own layout of actions within them and I need to be able to put the Options dropdown, New User button, FilterTextbox (and friends), the "7 users selected" message w/ appropriate count variable passed for the number, and the pagination buttons & messaging all in the same place. 4) Please proceed to make changes to the code base updating it per what I've said here. Ask any questions you have as you proceed. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…tContainerWithbackend
082c044 to
4503c23
Compare
| <PaginationActions | ||
| v-model="currentPage" | ||
| class="top-pagination-actions" | ||
| :itemsPerPage="itemsPerPage" | ||
| :totalPageNumber="totalPageNumber" | ||
| :numFilteredItems="downloadItemListLength" | ||
| > | ||
| /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to change the pagination layout for all pages? 👀 (Seems like a non-trivial design change so just flagging it for all relevant people to aggree/be informed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having it top & bottom made more sense when I had a full page of downloads and had to scroll 30 of them to the bottom but when it's empty like that it looks goofy 😅 - I've reverted it to the original.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @nucleogenesis! Just a few comments I noticed in a first read through
kolibri/plugins/facility/assets/src/views/users/common/UsersTable.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/users/common/UsersTable.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/users/common/UsersTableToolbar.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/facility/assets/src/views/users/UsersRootPage/index.vue
Outdated
Show resolved
Hide resolved
kolibri/plugins/learn/assets/src/my_downloads/views/DownloadsList/index.vue
Outdated
Show resolved
Hide resolved
| </KGrid> | ||
|
|
||
| <div class="flex-column"> | ||
| <div class="flex-column table-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like with the new changes this component is no longer necessary yes, or at least it should be renamed in the scope of this PR so we can avoid future confusions.
NOTE: This will likely all be overwritten in a future issue once the text is discussed in more detail
llm disclosure: portions of these changes made w/ help of Claude code
|
@pcenov I've updated the PR to address the following:
The following we'll address separately:
|
|
Hi @nucleogenesis, Testing notes on the fixed items:
New issues:
tooltips.mp4
search.field.disappears.mp4
search.field.size.mp4
small.devices.mp4 |
fixes tooltips hiding behind table headers
|
@pcenov I've fixed:
The others will be addressed in follow-ups - particularly the styles on the mobile screens |
|
@nucleogenesis - I'm not seeing a new build with the new fixes? |
|
Thanks @nucleogenesis - please let me know when you create the follow-up issues so that we can make sure that everything is tracked. filters.no.search.field.mp4Didn't observe any other issues caused by the changes made here. |
only hiding them if there are no searchterm, filters applied, or users in the table. honestly an edge case but now it works as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments.
I think this is good cleanup of the pagination - and reminds me somewhat of some of the composable work that @AlexVelezLl did in 0.18 for better handling for API requests for content. I think we are feeling our way towards a better, composable based, API resource layer that handles both cursor based (more) and page based pagination styles. Good that we didn't go that far in this PR, but it's moving us in the at direction.
The appBarHeight stuff feels like it could be avoided - I don't know precisely how, but I'd rather look for an alternative, and then clearly document why this was required. I guess the main question is, why isn't the built in paddingTop styling that is applied sufficient? If it's being ignored somehow, shouldn't we just be aiming to apply it in the correct place instead?
| </KGrid> | ||
|
|
||
| <div class="flex-column"> | ||
| <div class="flex-column table-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it feels like it would be better to just defer most of this to the consuming component, until we have a strong use case to generalize again.
| id="main" | ||
| class="main-wrapper" | ||
| :style="[wrapperStyles, paddingTop]" | ||
| :style="[wrapperStyles]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like this doesn't even need to be an array any more? Just the wrapperStyles object directly?
| * Handles page changes, items per page, and URL query synchronization | ||
| */ | ||
| export default function usePagination({ usersCount, totalPages } = {}) { | ||
| const route = useRoute(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice to do this, so as we drop use of vuex, we're not relying on store vuex state for this any more.
|
So, I think my general feeling after the last few updates is that the smell was coming from under the floorboards. Still good to have reverted the expansion of the API, because I think doing this right will not require the appBarHeight - the small is still there, it's just now masked again! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All code review and QA comments have been addressed, either in the scope of this PR or in follow up issues.


Summary
Major Changes
References
Touches #13782 , follows up from #13743
Reviewer guidance
QA
There are no specific Figma references for these updates to reference.
Users Root Table, New Users Table, Deleted Users Table
All of these pages have had similar changes performed on them and there is significant change for regression - so please check the following:
Root Table Specific
Android & App mode
Test that all content and data is visible when in app mode (where there is a bottom actions bar).
My Downloads
Check this page for regressions, particularly around pagination.
Classes -> Class Details -> Enroll learners
This page also has had some updates to the pagination and should be tested for regressions with a focus on pagination