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

Make KRadioButton to show a warning for developers when it's not nested inside KRadioButtonGroup #761

Open
4 tasks
MisRob opened this issue Aug 29, 2024 · 8 comments
Assignees
Labels
Component: KRadioButton Component: KRadioButtonGroup good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P2 - normal Priority: Nice to have TAG: a11y TAG: dev experience

Comments

@MisRob
Copy link
Member

MisRob commented Aug 29, 2024

🌱 Are you new to the codebase? Welcome! Please see the contributing guidelines.

Summary

In #650, we introduced a new KRadioButtonGroup that ensures KRadioButton groups are accessible in all supported browsers. For this to properly work, all KRadioButtons need to be nested inside KRadioButtonGroup like

<KRadioButtonGroup>
  <KRadioButton>
  <KRadioButton>
</KRadioButtonGroup>

but it could also be more deeply nested structures such as

<KRadioButtonGroup>
  <KGridItem>
    <div>
      <KRadioButton>
      <KRadioButton>
    </div>
  </KGridItem>
</KRadioButtonGroup>

The goal of this issue is to add logic to KRadioButton that shows a warning for developers when it's not nested inside KRadioButtonGroup.

The Value Add

This will notify developers about the proper usage and ensures expected a11y behavior for users.

Possible Tradeoffs

A risk of possible performance issues since it adds logic to every KRadioButton that needs to traverse all its parents (see "Guidance" on how to possibly take care of this)

Guidance

  • You can use the playground page for development and testing
  • KRadioButton documentation page with more examples of usage
  • As demonstrated in the examples above, note that we can't know how deeply KRadioButton is nested in KRadioButtonGroup so we will need to look at its chain of parents until KRadioButtonGroup is found.
    • However, as this will be run from every KRadioButton, we need to be careful to not use techniques of accessing the DOM that may cause performance issues.
    • Recursion over parents in this.$parent and checking each $parent's $options._componentTag until we find 'KRadioButtonGroup' component tag or until there is no more parents, could be the way
  • Lastly, there's no need to run the check in production so let's wrap it in if (process.env.NODE_ENV !== 'production') {. @rtibbles says "this will also tree shake the code out in webpack builds." ;)

Acceptance criteria

  • No regressions in KRadioButtons
  • Optimized for performance
  • Executes in the development mode only
  • Works for one level of nesting as well as multiple levels and has unit tests for these two scenarios

Comments

This update would ideally be released after learningequality/kolibri#12596 so that we're not overwhelmed by warnings in Kolibri ;)

@MisRob MisRob added Component: KRadioButton good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P2 - normal Priority: Nice to have Component: KRadioButtonGroup TAG: dev experience TAG: a11y labels Aug 29, 2024
@lokesh-sagi125
Copy link
Contributor

hey @MisRob can i work on this issue?

@MisRob
Copy link
Member Author

MisRob commented Sep 3, 2024

Hi @lokesh-sagi125, you're welcome to take this on. Thanks for your continued contributions. Please follow guidance closely and you're welcome to ask questions here if there's anything that's not clear.

@lokesh-sagi125
Copy link
Contributor

hey @MisRob you want the warning to be displayed in the console right?

@MisRob
Copy link
Member Author

MisRob commented Sep 4, 2024

Hi @lokesh-sagi125, yes

@iamshobhraj
Copy link

@MisRob can i work on this?

@AlexVelezLl
Copy link
Member

Hey @iamshobhraj! Thanks for your interest in contributing to this issue. Unfortunately, this is already assigned. But you’re welcome to find a "help wanted" issue with no assignee 🤗.

@lokesh-sagi125
Copy link
Contributor

hey @MisRob sorry for the inactivity i was busy with a hackathon ; i just opened a PR for this issue could you please review the changes :).

@AlexVelezLl
Copy link
Member

Closed by #781

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: KRadioButton Component: KRadioButtonGroup good first issue Self-contained, straightforward, low-complexity help wanted Open source contributors welcome P2 - normal Priority: Nice to have TAG: a11y TAG: dev experience
Projects
None yet
Development

No branches or pull requests

4 participants