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

tabs: better event and active attribute #232

Merged
merged 3 commits into from
Mar 22, 2024

Conversation

dbil25
Copy link
Contributor

@dbil25 dbil25 commented Mar 21, 2024

this PR

  • Adds an active="true" attribute to the active tab. Useful when you want some other javascript to we aware of which tab is active
  • Rename the dispatched event (might be breaking if some users relied on it) to be stimulus-y but using the stimulus dispatch function that will send the event tabs:tab-changed with event.target beeing the tab element itself and event.detail beeing { activeIndex: n}

@excid3
Copy link
Owner

excid3 commented Mar 21, 2024

Does the existing data-index-value for the current tab do the job of active="true" it may not be as convenient, but it's already there for that purpose.

Switching to this.dispatch() is a good idea. 👍

@dbil25
Copy link
Contributor Author

dbil25 commented Mar 21, 2024

Having the active attribute on the tab itself allows me to do some tailwind shenanigans like group-[&[active]]:some-cool-class it could also be a data-attribute like data-active if you think its more appropriate

@excid3
Copy link
Owner

excid3 commented Mar 22, 2024

Probably best to stick with data-active="true" but that seems like good enough reasoning to me.

@dbil25 dbil25 force-pushed the add-active-to-tabs branch from 569906b to 8eb4eb9 Compare March 22, 2024 02:28
@dbil25
Copy link
Contributor Author

dbil25 commented Mar 22, 2024

there you go

@excid3
Copy link
Owner

excid3 commented Mar 22, 2024

Thanks @dbil25 👍

@excid3 excid3 merged commit d5b3935 into excid3:main Mar 22, 2024
1 check passed
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