Tabs component #169
Tabs component #169
Conversation
| @@ -0,0 +1,5 @@ | |||
| <div data-test-ember-headlessui-tabs-wrapper ...attributes> | |||
There was a problem hiding this comment.
do we have to have this wrapping div?
| </div> | ||
| <div class="mx-auto w-full max-w-md shadow-md px-4 py-4 rounded-xl bg-white mt-4"> | ||
| {{#each this.contents as |content index|}} | ||
| <tabs.Content class="content" @content-index={{index}}> |
There was a problem hiding this comment.
can we remove the index? and compare via object identity or some other way? using indexes makes looping inefficient / invalidate more frequently than we need it to
| @@ -0,0 +1,22 @@ | |||
| <div class='w-full max-w-md px-2 py-16 mx-auto'> | |||
| <TabsGroup class="flex flex-col space-x-1 rounded-xl p-1" @onChange = {{this.onChange}} as |tabs|> | |||
There was a problem hiding this comment.
What are your thoughts on internalizing the onChange behavior by default?
can we abstract away the need for the consumer to configure this? (I think so!)
| @tracked currentTab = null; | ||
| @action | ||
| registerTabNames(e) { | ||
| this.tabNames = [...this.tabNames, e]; |
There was a problem hiding this comment.
instead of using immutability patterns, can we switch to a TrackedArray? that will provide more optimal updates and not invalidate the whole collection
| @@ -0,0 +1,7 @@ | |||
| <div class="{{if this.renderContent '' 'hidden'}}" | |||
There was a problem hiding this comment.
I don't think we can assume a "hidden" class exists
| get renderContent() { | ||
| return this.element === this.args.selectedContent; | ||
| } | ||
| registerContent = modifier((e) => { |
There was a problem hiding this comment.
can we not have this side-effect here? I know this pattern is common in headlessui, but it's a problematic pattern, and we don't want to propagate it.
Derived data will be more performant, have fewer layout shifts, and will in general be way easier to debug.
| id={{guid}} | ||
| disabled={{if @disable true false}} | ||
| {{on 'click' this.selectTab}} | ||
| {{this.registerTabs}} |
There was a problem hiding this comment.
remove this modifier please <3
There was a problem hiding this comment.
First off, thanks for submitting this!!! It's a huge help!
Only got part way through so far, but some general notes:
- don't have effects, instead, favor derived data patterns
- no registerItem/tab/whatever helpers/modifiers/etc
- I think we can reduce the amount of glue code consumers need to use
implemented tabs component
