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

[DO NOT MERGE BEFORE NEW KDS RELEASE][GSoC] Use KTable in Facility -> Classes #12571

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

BabyElias
Copy link

Summary

This PR introduces KTable component to Kolibri as a part of GSoC Project : Accessible Sortable Table.
The component is still under review in KDS.

Reviewer guidance

  • Navigate to [root]/en/facility/#/classes
  • Sign in if prompted
  • Click on the New Class button
  • Add a random class. Once added, the information is displayed using KTable on the classes page.
  • Add a few more random classes using New Class button to check functionalities of the table component.

QA Testing Guidance

  • Table navigable using both arrow keys and tab key.
  • When navigating any cell using arrow key or tab key, the corresponding column header and entire row containing the cell are highlighted.
  • Elements within cells that are focusable, receive proper focus using tab key and can be executed using 'Enter' key
  • The headers can be sorted by pressing 'Enter' key on them
  • The focus ring is visible clearly around the focused elements.
  • When viewport is shrinked for smaller devices, navigation using arrow keys/tab key should ensure that each element is clearly visible when navigating through them and does not get hidden by sticky headers.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@github-actions github-actions bot added APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend labels Aug 18, 2024
@MisRob MisRob changed the base branch from gsoc-table to develop August 18, 2024 13:02
@radinamatic
Copy link
Member

This is looking great @BabyElias !!! 👏🏽
Table navigation is fluid, styling as per requirements, activating the Delete class button works, although in one instance it displayed this error in the console:

TypeError: Cannot read properties of undefined (reading '3')

2024-08-20_14-30-10

One possible improvement would be in the area of how we notify that the columns are sortable, and after they have been sorted. Let's discuss if we could take some inspiration from examples here.

Copy link
Member

@LianaHarris360 LianaHarris360 left a comment

Choose a reason for hiding this comment

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

The KTable looks great, I'm so excited to see it used in Kolibri! I only had a few notes to make.

@MisRob MisRob self-assigned this Aug 27, 2024
@MisRob
Copy link
Member

MisRob commented Sep 4, 2024

@radinamatic mentioned to me that she re-tested and it all worked smooth so in her absence, I am leaving this note as QA approval :) Regarding the improvements suggested in https://dequeuniversity.com/library/aria/table-sortable, Radina is going to clarify the requirements at some point and then we will open a follow-up issue.

@MisRob MisRob changed the title [GSoC] KTable Trial for use-case 1 [DO NOT MERGE BEFORE NEW KDS RELEASE][GSoC] Use KTable in Facility -> Classes Sep 4, 2024
@@ -21,7 +21,8 @@
"js-cookie": "^3.0.5",
"knuth-shuffle-seeded": "^1.0.6",
"kolibri-constants": "0.2.6",
"kolibri-design-system": "5.0.0-rc2",
"kolibri-design-system": "https://github.com/BabyElias/kolibri-design-system#7764ef4b5f9b66ebd90d2ad389ae3d44252437a0",
Copy link
Member

Choose a reason for hiding this comment

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

Before merging this PR, we will replace this by the newest KDS release that will contain learningequality/kolibri-design-system#727

@@ -24,7 +24,7 @@
"js-cookie": "^3.0.5",
"knuth-shuffle-seeded": "^1.0.6",
"kolibri-constants": "0.2.6",
"kolibri-design-system": "5.0.0-rc2",
"kolibri-design-system": "https://github.com/BabyElias/kolibri-design-system#7764ef4b5f9b66ebd90d2ad389ae3d44252437a0",
Copy link
Member

Choose a reason for hiding this comment

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

Here too will be updated

Copy link
Member

@MisRob MisRob left a comment

Choose a reason for hiding this comment

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

@BabyElias Code-wise, all seems to be ready to me too. We just need to pull in the latest KDS release with the KTable as soon as it's released.

@MisRob
Copy link
Member

MisRob commented Sep 12, 2024

We can do final QA here, but the KDS upgrade as a whole will be handled in #12651 (the test failures should be resolved by that as well as some other breaking changes)

@radinamatic
Copy link
Member

As we mentioned during the weekly call, the only remaining issue would be to correct the z-order position of the first cell in the column header (issue visible when the page is scrolled on small screens):

first-cell

Otherwise, this should be good to go! 👏🏽 💯 🚀 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Facility Re: Facility App (user/class management, facility settings, csv import/export, etc.) DEV: frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants