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

Issue 10254: Improve coach tabs accessibility of Reports Lesson Tab #11606

Conversation

muditchoudhary
Copy link
Contributor

Summary

Use KTabs to improve the accessibility of the Reports Lesson Tabs.

Peek 2023-12-09 17-21

References

Closes #10254

Reviewer guidance

To review, Go to Coach -->Select a Class --> Reports --> Lesson --> Select a lesson. Then test the tabs with keyboard tabs key.

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

  • Automated test coverage is satisfactory
  • 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: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) SIZE: medium labels Dec 9, 2023
@muditchoudhary
Copy link
Contributor Author

Hi, @MisRob Another one has been fixed! Please review it.

I think all the mentioned follow-up issues are fixed. Yayy 🎉

@MisRob MisRob self-requested a review December 11, 2023 07:21
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.

I think all the mentioned follow-up issues are fixed. Yayy 🎉

Yes, amazing!

Thank you @muditchoudhary, code is looking good and it seems to work well too. I added one non-blocking, nice-to-have, suggestion. And I'll also ask @radinamatic to test before we merge.

@@ -227,6 +271,12 @@
}
},
},
$trs: {
coachReportsLesson: {
Copy link
Member

Choose a reason for hiding this comment

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

Apologies I forgot to mention this in previous work (and probably also use myself in the PR I recommended as a template). We're migrating away from placing strings into components. Could you please move this string to commonCoachStrings and import it from there?

In regards to importing the string, we'd appreciate if you could use this fairly new way of using setup() https://kolibri-dev.readthedocs.io/en/develop/i18n.html?highlight=translate#common-string-modules. Even though you may see translation mixins being used all over the codebase, whenever possible we'd like to use this pattern for all new code additions.

@muditchoudhary
Copy link
Contributor Author

muditchoudhary commented Dec 14, 2023

Okay I'll have a commit soon. Please wait till then before merging.

@MisRob
Copy link
Member

MisRob commented Dec 15, 2023

Thank you @muditchoudhary! I also wanted to mention I won't be able to follow-up here this year as this is my last day before few weeks of time off. Someone else may jump in, but the whole team's availability will too soon be limited until the second week of January. Thank you and hopefully see you next year :)!

@MisRob MisRob requested review from akolson and removed request for akolson December 15, 2023 09:39
@radinamatic
Copy link
Member

Any reason why is this PR targeted to develop instead of 0.16, as the previous one that added the same change (as far as I understand) to quiz reports?

@MisRob
Copy link
Member

MisRob commented Dec 15, 2023

Any reason why is this PR targeted to develop instead of 0.16, as the previous one that added the same change (as far as I understand) to quiz reports?

@radinamatic Yes, the original intention was to target develop with all of new work. Majority of the PRs from this group target it. But the previous PR you mentioned was easier to work on the release branch because the place was broken due to some ongoing coach features.

@MisRob
Copy link
Member

MisRob commented Dec 15, 2023

the place was broken due to some ongoing coach features

More accurately, a contributor couldn't create data because of broken quiz creation and therefore it was impossible to navigate to the place they needed to update because it assumed existence of at least one quiz, I believe

@muditchoudhary this is not related to your work

@marcellamaki marcellamaki self-requested a review December 19, 2023 16:29
@muditchoudhary muditchoudhary force-pushed the issue-10254-improve-coach-tabs-accessibility branch from acb9036 to e727dd0 Compare December 21, 2023 11:35
@muditchoudhary
Copy link
Contributor Author

There was a conflict. I resolved it

@muditchoudhary
Copy link
Contributor Author

Please review it. I do not exactly understand how we use string from a common module with setup.

@MisRob
Copy link
Member

MisRob commented Jan 9, 2024

Thank you, @muditchoudhary. I've just returned from my vacation and will have a look at it again in the next couple of days.

@muditchoudhary
Copy link
Contributor Author

Thank you, @muditchoudhary. I've just returned from my vacation and will have a look at it again in the next couple of days.

Okay. Alright. I hope you had a great vacation. Until then I'll look into some new issues. Thank you!

@MisRob
Copy link
Member

MisRob commented Jan 11, 2024

The latest updates are looking good to me, thank you @muditchoudhary. @radinamatic could you please try this out?

@MisRob MisRob assigned MisRob and unassigned MisRob Jan 16, 2024
@MisRob
Copy link
Member

MisRob commented Jan 19, 2024

Hi @muditchoudhary, we'll get to test and merge this, we just need to prioritize QA of the release branch rather than the develop right now. You can consider it accepted :)

@muditchoudhary
Copy link
Contributor Author

Hi @muditchoudhary, we'll get to test and merge this, we just need to prioritize QA of the release branch rather than the develop right now. You can consider it accepted :)

Okay, got it. Thank you for letting me know.

@radinamatic
Copy link
Member

radinamatic commented Feb 12, 2024

Installed the EXE asset from this PR on Windows 10 in both Chrome and Firefox, but I'm unable to open a report for a lesson, using neither the keyboard, nor the mouse. Report for quiz apparently opens correctly.

Firefox Chrome
https://github.com/learningequality/kolibri/assets/1457929/514368f3-f51b-4641-be85-df9c92bcaf66 2024-02-12_14-15-50

@muditchoudhary
Copy link
Contributor Author

Installed the EXE asset from this PR on Windows 10 in both Chrome and Firefox, but I'm unable to open a report for a lesson, using neither the keyboard, nor the mouse. Report for quiz apparently opens correctly.

Firefox Chrome
https://github.com/learningequality/kolibri/assets/1457929/514368f3-f51b-4641-be85-df9c92bcaf66 2024-02-12_14-15-50

Currently working on this: #10491 I'll check it after completing it.

@MisRob
Copy link
Member

MisRob commented Feb 13, 2024

I think it will have something to do with the new way of using strings I recommended. I see you followed the documentation so if you don't mind, I would have a look @muditchoudhary rather sooner what's going on so we can eventually fix it (we'd like to use strings in this way in other work)

@MisRob MisRob assigned MisRob and unassigned radinamatic Feb 13, 2024
@muditchoudhary
Copy link
Contributor Author

I think it will have something to do with the new way of using strings I recommended. I see you followed the documentation so if you don't mind, I would have a look @muditchoudhary rather sooner what's going on so we can eventually fix it (we'd like to use strings in this way in other work)

okay 👍

@MisRob
Copy link
Member

MisRob commented Feb 15, 2024

@muditchoudhary We have a typo in documentation. I fixed it in this PR and will open a PR to fix documentation too. @radinamatic you can now test the page, thank you.

@MisRob MisRob mentioned this pull request Feb 15, 2024
9 tasks
@radinamatic
Copy link
Member

Tested with NVDA in Firefox on Windows 10, looking much better now! 👏🏽

Keyboard navigation NVDA speech viewer
reports-lessons 2024-02-15_18-44-21

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Manual QA & A11y testing passed, yey for more accessible tabs in Kolibri! 💯 👏🏽 :shipit:

Thank you for your contribution @muditchoudhary!

@MisRob
Copy link
Member

MisRob commented Feb 15, 2024

Yey! Thanks @radinamatic and @muditchoudhary!

@MisRob
Copy link
Member

MisRob commented Feb 15, 2024

Docs failure is not related to this PR and should be fixed on develop, so merging.

@MisRob MisRob merged commit b10ab0b into learningequality:develop Feb 15, 2024
30 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Coach Re: Coach App (lessons, quizzes, groups, reports, etc.) DEV: frontend SIZE: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Coach tabs accessibility - "Report-Learners" tabs on a Reports lesson page
3 participants