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

Epub viewer settings side bar redesigned #11156

Merged

Conversation

Akila-I
Copy link
Contributor

@Akila-I Akila-I commented Aug 25, 2023

Summary

With the user customized themes introduced in the epub viewer, the name of the themes is identified as an important attribute to be showed to the user. Also the compact view of the DELETE and EDIT buttons associated with each custom theme makes the settings side bar UI look overpopulated with items in it.

This PR refers to the designs from the Kolibri design team. As the first step of implementing the new design, The settings side bar view is modified to give user a simple and intuitive set of items to navigate through.

Also the focus management in the Modals to select different colors for the themes is implemented as well.

This PR is followed by the changes introduced in #11041 .

Screenshots

  • Before (Desktop view)
    image

  • After (Desktop view)
    image

  • Before (Mobile view)
    image

  • After (Mobile view)
    image

References

Figma design for the new EPUB viewer settings : https://www.figma.com/file/zpwcUcGR8D0hKUGvZU8kf3/Enhanced-EPUB-Renderer?type=design&node-id=0%3A1&mode=design&t=CgU7rA96zHncjSwZ-1

Reviewer guidance

Followings can be experienced in reviewing:

  • Focus management of the Modals
  • New Settings SideBar view

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

@radinamatic
Copy link
Member

Kudos @Akila-I , this looks good! 👏🏽

Keyboard navigation is very smooth, and thank you so much for implementing the additional context in the Delete and Edit buttons for custom themes, so the user knows exactly that their focus ends on the buttons for a specific theme for deleting and editing 🎉 😍

2023-08-28_0-53-16

I've run the accessibility checker after testing with the screen reader, just to be sure that I covered all the angles. It revealed several issues that would be important in some formal accessibility audit, but all pre-existing and none relevant to your GSoC work. One single point for improvement that I would recommend at this point, given the low push, is to add the missing aria labels for the two current themes (yellow and blue). As you can see, both are missing the aria-label that are present in the first 4 default themes:

2023-08-28_0-22-57

@Akila-I
Copy link
Contributor Author

Akila-I commented Aug 28, 2023

Kudos @Akila-I , this looks good! 👏🏽

Keyboard navigation is very smooth, and thank you so much for implementing the additional context in the Delete and Edit buttons for custom themes, so the user knows exactly that their focus ends on the buttons for a specific theme for deleting and editing 🎉 😍

2023-08-28_0-53-16

I've run the accessibility checker after testing with the screen reader, just to be sure that I covered all the angles. It revealed several issues that would be important in some formal accessibility audit, but all pre-existing and none relevant to your GSoC work. One single point for improvement that I would recommend at this point, given the low push, is to add the missing aria labels for the two current themes (yellow and blue). As you can see, both are missing the aria-label that are present in the first 4 default themes:

2023-08-28_0-22-57

Thank you for the feedback @radinamatic , sure I can add the missing labels for existing themes.
Would you think we can push these commits on top of the PR ( #11041 ) that we are currently reviewing to merge?

@radinamatic
Copy link
Member

Since adding those missing label would be changes independent from your current work, it should be safe to add in #11041 👍🏽

@Akila-I
Copy link
Contributor Author

Akila-I commented Aug 28, 2023

Since adding those missing label would be changes independent from your current work, it should be safe to add in #11041 👍🏽

I think I might first update the design for the mobile view as well. so that we can merge a responsive UI feature

@Akila-I Akila-I changed the base branch from develop to epub-custom-themes August 29, 2023 07:24
@Akila-I Akila-I changed the title Epub redesigned - created for test the build artefacts Epub viewer settings side bar redesigned Aug 31, 2023
@radinamatic
Copy link
Member

Retested with NVDA after the latest commits, and all the previous suggestions and points of improvement have been addressed.

2023-09-06_15-27-17

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.

This is formally still a draft, but looks good to go! 🙂 💯 :shipit:

@Akila-I Akila-I marked this pull request as ready for review September 6, 2023 14:55
@Akila-I
Copy link
Contributor Author

Akila-I commented Sep 6, 2023

This is formally still a draft, but looks good to go! 🙂 💯 :shipit:

Made it ready to review. Thanks for the feedback

@marcellamaki marcellamaki self-requested a review September 6, 2023 15:42
Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Hi - looks great! I added just a few very small comments for readability if you're able to get to them. Overall, really good work! Excited to get this merged :) I think it should be good to go as soon as these updates are in.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Thanks @Akila-I! 🎉

@marcellamaki marcellamaki merged commit 9cb8a25 into learningequality:epub-custom-themes Sep 7, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants