Skip to content

feat: add tabindex input and binding #438

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 1 commit into from
May 15, 2024

Conversation

ConnorSmith-pf
Copy link
Contributor

@ConnorSmith-pf ConnorSmith-pf commented May 11, 2024

Adds tabindex Input for split component & defaults to previous fixed value of 0.

Closes #432

@Jefiozie Jefiozie requested review from SanderElias and Jefiozie May 15, 2024 01:12
Copy link
Member

@Jefiozie Jefiozie left a comment

Choose a reason for hiding this comment

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

Lgtm

@Jefiozie Jefiozie merged commit 1d259cf into angular-split:main May 15, 2024
5 of 6 checks passed
@Harpush
Copy link
Collaborator

Harpush commented May 15, 2024

@SanderElias what about documentation of the new input?

@Harpush
Copy link
Collaborator

Harpush commented May 15, 2024

@SanderElias And one more thing - by calling the input tabindex won't it apply tabindex to the whole split? Shouldn't it be gutterTabIndex.
What's more I can't see it usable unless set to -1. Shouldn't it actually be more like disableGutterTabindex boolean input?

@SanderElias
Copy link
Contributor

@Jefiozie @Harpush is right! We should unmerge this one ;)
I didn't think this one through. Indeed -1 is the only valid option for the gutter (besides the default 0)
And the only reason to set it is if you want to take the gutter out of the flow of the page. I'm in favor of the disableGutterTabindex boolean input

@SanderElias
Copy link
Contributor

the question, and I don't know the answer, but I'm leaning toward no.
Should the gutter receive tab-index by default?
@Harpush @Jefiozie @ConnorSmith-pf ?

Jefiozie added a commit that referenced this pull request May 15, 2024
@Jefiozie
Copy link
Member

I agree on leaning NO, I made a PR for the revert #440

@ConnorSmith-pf
Copy link
Contributor Author

False would keep it in line with current behaviour (i.e. keeping it in the flow). I think I agree with being it false to begin with.

@Harpush
Copy link
Collaborator

Harpush commented May 15, 2024

@Jefiozie the gutter should have 0 by default I think and will be -1 if provided true. So the default for the input is indeed false. Maybe add a global config for it? Might be overkill though.

@SanderElias
Copy link
Contributor

SanderElias commented May 16, 2024

False would keep it in line with current behaviour (i.e. keeping it in the flow). I think I agree with being it false to begin with.

Yes, but in our first breaking release, we should toggle this.

@Harpush
Copy link
Collaborator

Harpush commented May 16, 2024

False would keep it in line with current behaviour (i.e. keeping it in the flow). I think I agree with being it false to begin with.

Yes, but in our first breaking release, we should toggle this.

I am not entirely sure about it... Without it for example the keyboard gutter movement feature won't work

Jefiozie added a commit that referenced this pull request May 20, 2024
@RicardoTactiql
Copy link

Why was this reverted? How can I set it to -1?

@Harpush
Copy link
Collaborator

Harpush commented Sep 6, 2024

Why was this reverted? How can I set it to -1?

I reopened the issue

@ConnorSmith-pf ConnorSmith-pf deleted the 432-tab-index branch November 25, 2024 12:24
@Jefiozie
Copy link
Member

Jefiozie commented Feb 2, 2025

@all-contributors please add @ConnorSmith-pf for code.

🤖 Beep boop! This comment was added automatically by all-contributors-auto-action.
Not all contributions can be detected from Git & GitHub alone. Please comment any missing contribution types this bot missed.
...and of course, thank you for contributing! 💙

Copy link
Contributor

@Jefiozie

I've put up a pull request to add @ConnorSmith-pf! 🎉

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.

Input to control tabindex
5 participants