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: prevent focus on first render #1383

Merged
merged 11 commits into from
Dec 13, 2022
Merged

fix: prevent focus on first render #1383

merged 11 commits into from
Dec 13, 2022

Conversation

felix-ico
Copy link
Collaborator

No description provided.

@render
Copy link

render bot commented Nov 24, 2022

@felix-ico
Copy link
Collaborator Author

fixes #1372.

not sure if the new prop autoFocus should be true or false by default? If we want to keep the previous behavior (first element focussed) I should change it to true, if we want it to behave as the issue reporter wishes we can keep it with false as it is implemented here.

@felix-ico
Copy link
Collaborator Author

@acstll could you review this?

Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

Hey @felix-ico thanks for taking care of this one! It's working as expected now 👍 …but I think we shouldn't be exposing a firstRender attribute in Tab Header, it doesn't seem useful for consumers 🤔 — Could we make this something internal?

@felix-ico
Copy link
Collaborator Author

@acstll i removed the stencil prop and used a regular attribute instead, seems to work as expected.

@felix-ico felix-ico changed the title fix: add autoFocus prop fix: prevent focus on fisrt render Nov 30, 2022
@felix-ico felix-ico changed the title fix: prevent focus on fisrt render fix: prevent focus on first render Nov 30, 2022
Copy link
Collaborator

@acstll acstll left a comment

Choose a reason for hiding this comment

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

👌

@felix-ico felix-ico merged commit b455cda into main Dec 13, 2022
@felix-ico felix-ico deleted the fix/1372-tab-nav-focus branch December 13, 2022 15:37
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.

2 participants