Skip to content

Conversation

muildrik
Copy link
Contributor

… I have also updated the viewer.mdx file with the specifics. This implementation simply passes css down to the component as suggested here.

… I have also updated the viewer.mdx file with the specifics. This implementation simply passes css down to the component as suggested here: https://stackoverflow.com/questions/72419247/how-can-a-prop-value-be-used-in-an-stitches-js-component
Copy link
Member

@mathewjordan mathewjordan left a comment

Choose a reason for hiding this comment

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

Hey @muildrik. Thank you for this contribution.

I am not completely opposed to Tabs as a "stack" for a vertical option, but I feel like an Accordion is really what could improve the UX here. https://www.radix-ui.com/primitives/docs/components/accordion and would love to see that explored.

Stacking vertical tabs above content rather than to the aside seems like not the best UX. The wrap of a tabs in row is an obvious improvement here, however I think we should continue digging.

displaySettings = {
"--flex-direction": "row",
"--flex-wrap": "wrap",
"--justify-content": "space-between",
Copy link
Member

Choose a reason for hiding this comment

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

Is space-between necessary here? It creates an unnecessary gap between tabs.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"--justify-content": "space-between",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It uses all the space available so that at different screen sizes with two options on the same line will always be aligned the same manner (rather than have a big gap at one end or the other). I use space-between quite often for that reason, but purely a personal choice--easy enough to drop it!

renderToggle: true,
renderAnnotation: true,
renderContentSearch: true,
tabLayout: "stack",
Copy link
Member

Choose a reason for hiding this comment

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

stack as the default seems like a big shift. I'd opt for row as it's not a breaking change in layout.

Suggested change
tabLayout: "stack",
tabLayout: "row",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

| `options.informationPanel.renderToggle` | `boolean` | No | true |
| `options.informationPanel.renderContentSearch` | `boolean` | No | true |
| `options.informationPanel.defaultTab` | `string` | No | |
| `options.informationPanel.tabLayout` | `string` | No | stack |
Copy link
Member

Choose a reason for hiding this comment

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

I feel like we should consider accordion vs tabs here. Vertical tabs as a component often run to the aside of their content, not above them. I understand that's a bigger shift in functionality, however, it feels awkward to rush in this without consideration to the role of the component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, an accordion sounds good too! The only consideration here I think is that navigation would take up a little more vertical space, which may not always be preferable? Dunno--let's try!

useEffect(() => {
if (informationPanel && informationPanel.tabLayout) {
let displaySettings = {};
switch (informationPanel.tabLayout) {
Copy link
Member

Choose a reason for hiding this comment

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

If we are to implement vertical tabbing, we should also make sure the orientation of the Tabs.Root is set to be vertical according to https://www.radix-ui.com/primitives/docs/components/tabs#root.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think vertical tabbing should only be used in scenarios where there is very limited horizontal space available unless we go full on with vertical navigation--horizontal navigation is what people are normally most-used to and I don't think we'd want to use them interchangeably/mix them up. How about I try and come up with something for Accordion and see how that works? If that works properly/we like that one we should probably stick to a fully-vertical navigation setup.

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