📑 @tournant/tabs #62
📑 @tournant/tabs #62
Conversation
marcus-herrmann
changed the title
marcus-herrmann
force-pushed
the
feature/component/add-tab-component
branch
from
3b1889d
to
259922c
Compare
| @@ -0,0 +1,7 @@ | |||
| Copyright 2019 Oscar Braunert <[email protected]> | |||
There was a problem hiding this comment.
Note to self: Need to update the license header in communard.
| </tournant-tabs-widget> | ||
| ``` | ||
| Finally, decide on the initial tab. Pass the tab/panel name into `<TournantTabsWidget>`'s initial prop. | ||
|
|
There was a problem hiding this comment.
I would love to have examples that got deeper than this. What happens if there is interactive contents, headline structure etc. Also, we should avoid foo, bar, baz as variable names. Rather come up with a simple real life example. Maybe cooking related?
There was a problem hiding this comment.
First Course, Second Course, Third Course, Dessert. I like it.
| @click="clickHandler" | ||
| @keydown="keydownHandler" | ||
| > | ||
| <slot name="tabs"></slot> |
There was a problem hiding this comment.
I wonder if we could simplify the implementation by using a prop instead of a slot here. Would probably also reduce the amount of code we would have to write in the event handlers down here. At the moment it seems like we miss to leverage most of the benefits Vue provides us for modifying attributes.
There was a problem hiding this comment.
Reasoning behind using <tournant-tab> was that that way, people could place more than just text in Tabs, for example an icon. This wouldn't be possible if tabs were an array, e.g. ["Foo", "Bar", "Baz"]. I know what you mean regarding this approach not being vue-ish, though. I'm wondering for quite some while now: is there a third way?
There was a problem hiding this comment.
Hmm … good point. Let me think about this. Of the top of my head: When using an array of objects we could provide some level of customisation (e.g. class names and text). If we change Tab.vue into a functional component we might(!) be able to provide an option to render child components through it (say you want to add an icon that is a component). Either way, we should be very clear what is and what is not usable (e.g. don’t put complex structures in there as the button will likely eat its semantics). Locking this down has some advantages, as keeping it open.
If we keep TournantTab, we have to make sure that the name for the ID is slugified. A random ID wouldn’t work here as Tab and Tabpanel need to have the same name, right?
In the end: It depends. Need to get those stickers now.
There was a problem hiding this comment.
If we keep TournantTab, we have to make sure that the name for the ID is slugified. A random ID wouldn’t work here as Tab and Tabpanel need to have the same name, right?
Correct.
| <slot name="tabs"></slot> | ||
| </div> | ||
| <div ref="panellist"> | ||
| <slot name="panels"></slot> |
There was a problem hiding this comment.
Slot here makes sense to provide a mechanism for implementors to add any kind of content.
There was a problem hiding this comment.
Is this actionable or rather thinking out loud? If the latter: Yes, that's what I was thinking.
There was a problem hiding this comment.
This was thinking out loud agreement :)
| keydownHandler: function(e) { | ||
| const activeElem = this.$refs.tablist.querySelector(`#${this.activeTab}`) | ||
| const activeIndex = Array.from(this.$refs.tablist.children).indexOf( | ||
| activeElem | ||
| ) | ||
| let targetTab | ||
| switch (e.keyCode) { | ||
| case 37: | ||
| if (activeIndex - 1 < 0) { | ||
| targetTab = this.tabs[this.tabs.length - 1] | ||
| } else { | ||
| targetTab = this.tabs[activeIndex - 1] | ||
| } | ||
| this.changeTab(targetTab.id) | ||
| break | ||
| case 39: | ||
| if (activeIndex + 1 > this.tabs.length - 1) { | ||
| targetTab = this.tabs[0] | ||
| } else { | ||
| targetTab = this.tabs[activeIndex + 1] | ||
| } | ||
| this.changeTab(targetTab.id) | ||
| break | ||
| default: | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Can we break this up into multiple methods for the single events?
# Conflicts: # ui/index.html
|
A case we probably also need to think about: How does it collapse in a «server side rendering but JS fails to run»-environment? |
This concern is valid for all of tournant, I'm afraid |
It indeed is. I’ll open an issue to explore the robustness of the components once implemented. Building the project website could help with this. 🙈 |
|
See #71 |
What has been added
How can this be tested
In which screen readers have these additions been tested
In which browsers have these additions been tested
Mobile
Desktop
Additonal remarks