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

fix(admin): DataTable replaced with useTable #478

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

Komal914
Copy link
Contributor

@Komal914 Komal914 commented Sep 12, 2023

Checklist:

Closes #467

Screenshot 2023-09-11 at 10 16 51 PM

In the past DataTable has caused issues so admin has been updated with useTable. Since the UI is slightly different, the snapshot needed to be updated for adminTable.

@Komal914 Komal914 requested a review from a team as a code owner September 12, 2023 05:13
@Komal914 Komal914 changed the title DataTable replaced with useTable fix(admin): DataTable replaced with useTable Sep 12, 2023
Copy link
Contributor

@ngillux ngillux left a comment

Choose a reason for hiding this comment

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

Tested the changes of this PR, everything seems to work correctly (CI checks have also passed), however, from a UI perspective - there is a missing footer (the image attached shows the current live state of the admin page that's on main / production / proper).

Screenshot 2024-01-21 at 2 22 38 PM

The footer allows for navigation between the number of entries in the list, as well as having the option to filter between 'n' amount of entries being displayed on the page.

Aside from that the expected behavior was achieved. @Komal914 would you be able to look at the previous admin table and compare it to your implementation and see if you can add the option to show 'n' amount of entries?

Or could you share if this is not possible to do with the new library, and if it needs to be implemented another way (i.e., custom footer component)? It's a minor detail but once that is established it would let us know what the next step is. If it is just not an option with the new library then I think this PR is okay to merge.

This PR was tested by me via navigating through the pages as the 'ADMIN' user and ensuring no code would break. Additionally, I navigated as a 'TEACHER' and 'STUDENT' and ensured these pages were functioning correctly as well. Also created classes and went through the navigation for that. Everything worked as anticipated.

@utsab

@Komal914
Copy link
Contributor Author

Komal914 commented Jan 22, 2024

Thanks for taking a look @ngillux , there is a way to implement the footer with the new library. I can research and try an implementation. I will say 'React Table' is a bit confusing so I was curious to see if there a better library. Additionally, the library also released an update to version 8. So we probably should use the latest version.

Also, seems that we are using the previous library with 'DataTable', in other parts of the application -> "/components/dashtable.js". Since we are replacing this library inside this PR, it would make sense to also update this section.

i think it might be valuable for us to create a re-usable component which we can import in to use as the base table. Or consider switching to a new library for tables, as 'React Table' version 8 seems to be all in typescript. I found this new library which seems to be very well documented.

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.

Replace DataTable with useTable inside adminTable.js
2 participants