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 KRadioButton autofocus on dynamic rendering #492

Merged
merged 5 commits into from
Nov 30, 2023

Conversation

AlexVelezLl
Copy link
Member

@AlexVelezLl AlexVelezLl commented Nov 23, 2023

Description

Add autofocus directive on KRadioButton to fix autofocus behavior on dynamic rendering.

Issue addressed

Closes #489

Before/after screenshots

Before

Compartir.pantalla.-.2023-11-16.09_06_25.mp4

After

Compartir.pantalla.-.2023-11-23.11_42_57.mp4

Changelog

  • #492
    • Description: Add autofocus directive on KRadioButton to fix autofocus behavior on dynamic rendering.
    • Products impact: Kolibri's Setup Wizard
    • Addresses: KRadioButton autofocus not working on dynamic rendering #489
    • Components: KRadioButton
    • Breaking: no
    • Impacts a11y: yes
    • Guidance: Add "autofocus" prop on KRadioButton. This change improves keyboard navigation on forms where a KRadioButton jumps into the DOM.

Steps to test

  1. Simulate a flow where a KRadioButton is dynamically rendered (for example, the steps on the setup wizard).
  2. Add the property "autofocus" to KRadioButton.
  3. The KRadioButton input should be focused when it appears in the DOM.

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical and brittle code paths are covered by unit tests
  • The change is described in the changelog section above

Reviewer guidance

  • Is the code clean and well-commented?

After review

  • The changelog item has been pasted to the CHANGELOG.md

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.

Thank you @AlexVelezLl. good work! Code seems to be fine. Just left one minor note about the changelog. I tested manually and confirm it works.

Is there a corresponding issue in Kolibri in which scope we would introduce this?

CHANGELOG.md Outdated
@@ -7,6 +7,17 @@ Changelog is rather internal in nature. See release notes for the public overvie

## Version 2.0.0

- [#492]
- **Description:** Add autofocus directive on KRadioButton to fix autofocus behavior on dynamic rendering.
- **Products impact:** Kolibri's Setup Wizard
Copy link
Member

Choose a reason for hiding this comment

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

Could you please change this to "bugfix"? This is not as much about the place it fixes, but rather a general category. There are some examples in the PR template you can choose from.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, hadn't noticed that. Thanks!

- **Components:** KRadioButton
- **Breaking:** no
- **Impacts a11y:** yes
- **Guidance:** Add "autofocus" prop on KRadioButton. This change improves keyboard navigation on forms where a KRadioButton jumps into the DOM.
Copy link
Member

Choose a reason for hiding this comment

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

Nice, thanks!

@MisRob
Copy link
Member

MisRob commented Nov 29, 2023

Also, one note on the process, after we merge a KDS PR, it is customary to update KDS version on Kolibri's develop branch (but not on the release branch) to the latest KDS commit

@AlexVelezLl
Copy link
Member Author

Yes, it should help address the issue Kolibri#11458, I already added the autofocus props in the PR Kolibri#11537.

@MisRob
Copy link
Member

MisRob commented Nov 29, 2023

Yes, it should help address the issue learningequality/kolibri#11458, I already added the autofocus props in the PR learningequality/kolibri#11537.

Ah great, in that case you can install this KDS version in your existing Kolibri PR. Let me know if you needed help.

@AlexVelezLl AlexVelezLl merged commit 321fd6c into learningequality:main Nov 30, 2023
8 checks passed
@AlexVelezLl AlexVelezLl deleted the kradio-fix-autofocus branch November 30, 2023 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KRadioButton autofocus not working on dynamic rendering
2 participants