Skip to content

Expose "selectTab" method for programmatic selection #56

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

Merged
merged 4 commits into from
Aug 24, 2022

Conversation

thetutlage
Copy link
Contributor

@thetutlage thetutlage commented Aug 10, 2022

This PR moves the selectTab method on the TabContainer class. Doing so will enable programmatic select of tabs by calling this method. The proposal was shared in this issue #55

Changes

  • The selectTab method no longer accepts the tabContainer instance. It uses this implicitly.
  • A conditional has been added to check for out of bounds index (since the method is public now). Right now, I am silently returning when the index > no of tabs. Maybe we can raise an exception here? Just wanted feedback on how to proceed with that.

Lemme know if something needs attention. Happy to improve the PR :)

Since the "selectTab" method is part of the class prototype, it is
now exposed as the public API, enabling to programmatically select
tabs.
@thetutlage thetutlage requested a review from a team as a code owner August 10, 2022 03:44
@thetutlage thetutlage requested a review from manuelpuyol August 10, 2022 03:44
src/index.ts Outdated
* Out of bounds index
*/
if (index > tabs.length - 1) {
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I think throwing a RangeError here would be very useful.

Copy link
Contributor

@keithamus keithamus left a comment

Choose a reason for hiding this comment

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

Looks great so far! Throwing would be helpful, and adding tests for out-of-bounds would be 😲 🎉

src/index.ts Outdated
* Out of bounds index
*/
if (index > tabs.length - 1) {
throw new Error(`Cannot select tab at index "${index}" as it exceeds the total number of tabs`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps a more succinct error message here:

Suggested change
throw new Error(`Cannot select tab at index "${index}" as it exceeds the total number of tabs`)
throw new RangeError(`Index "${index}" out of bounds`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure :)

@koddsson
Copy link
Contributor

This looks fine to me. I've got slight worries that this might regress the experience for screenreader users, but I couldn't find anything about if we should announce when the tab changes to users.

@thetutlage
Copy link
Contributor Author

Not an accessibility expert at all. But, shouldn't setting aria-selected and tab-index will take care of it?

My use case is to have a language switch on the website and once you select language (ie TypeScript or JavaScript), all the codeblocks on that page will switch to the selected language.

tabs-switch.mov

@thetutlage
Copy link
Contributor Author

Any feedback guys?

@keithamus keithamus requested review from dgreif and removed request for manuelpuyol August 24, 2022 09:46
@keithamus
Copy link
Contributor

Thanks for your patience @thetutlage, this slipped between the cracks of our first responder rotation. @dgreif would you kindly give this some extra 👀 and if you're happy merge+release?

@dgreif dgreif merged commit 1f03753 into github:main Aug 24, 2022
@thetutlage
Copy link
Contributor Author

Thanks :)

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

Successfully merging this pull request may close these issues.

4 participants