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

Adjust padding for visible focus outline on bottom bar buttons #9478

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Jun 2, 2022

Summary

This PR redistributes some of the the padding from the outer div of the BottomBar to an inner div so the focus ring is not cut off. It does not change the visual appearance of the spacing (it add up to the same amount for the component).

On the bar on the content renderer, it adds some padding, for the same purpose.

References

Fixes #9464
Fixes #9181

Selection of some pages with the changes applied

Page Screen
Content Renderer (Exercise) Screen Shot 2022-06-02 at 9 07 53 AM
Manage Channel Content Screen Shot 2022-06-02 at 9 08 25 AM
Manage Lesson Resources Screen Shot 2022-06-02 at 9 12 14 AM
Create Quiz Screen Shot 2022-06-02 at 9 14 05 AM
Taking a quiz Screen Shot 2022-06-02 at 9 16 21 AM

Reviewer guidance

I think since I made the changes to the bottom bar itself, most of the issues should be resolved. I cross referenced on multiple pages where it is implemented (see screenshots). However, it is possible there are cases that follow a similar pattern but do not use the BottomBar component (such as in the content page, where I added the changes separately).

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

@pcenov
Copy link
Member

pcenov commented Jun 3, 2022

LGTM - tested and verified as fixed in https://buildkite.com/learningequality/kolibri-python-package/builds/5624

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 by @pcenov passed, good to go! 💯 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants