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

Adding tabs #32

Closed
wants to merge 2 commits into from
Closed

Adding tabs #32

wants to merge 2 commits into from

Conversation

CaptainFalcon92
Copy link
Contributor

@CaptainFalcon92 CaptainFalcon92 commented Apr 22, 2020

This is a first try at reproducing W98 tabs - #21

I looked at how tabs behaves in W98 and it looks like normal tabs are justified left.
Then only when tabs become multi-rows then they are justified in full width.
Also i think i remember the row where belongs the active tabs is always pulled down, isn't it ?

Here are some single row tabs
Screenshot 2020-04-22 at 20 58 46
Screenshot 2020-04-22 at 20 59 13

And some multi-rows tabs
Screenshot 2020-04-22 at 20 56 51
Screenshot 2020-04-22 at 20 57 58

Here is the HTML

<menu class="tabs">
  <li class="active"><a href="#">Desktop</a></li>
  <li><a href="#">My computer</a></li>
  <li><a href="#">Control panel</a></li>
  <li><a href="#">Devices manager</a></li>
  <li><a href="#">Hardware profiles</a></li>
  <li><a href="#">Performance</a></li>
</menu>
<div class="window">
  <div class="window-body">
    <!-- the tab content -->
  </div>
</div>

For multi-row you can use .tabs.justified.
Although now i think it should be called .multirows.

Given i tried to reuse the .window as a tab container i had to find some hacks and made use of z-index in order to pull the active tab upfront and cover the container borders.
Is there a reason why the borders are made with box-shadows instead of real borders ?

Best 🙂

style.css Outdated
@@ -478,3 +483,45 @@ summary:focus {
width: 16px;
background-image: svg-load("./icon/button-right.svg");
}

menu.tabs {
Copy link
Owner

Choose a reason for hiding this comment

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

Incredible work 😍

While you're still hacking on this, wanna rework these selectors to guarantee proper aria attributes?

That is:

  • instead of .tabs we could do [role=tablist]
  • instead of .tabs li we could do [role=tab]
  • [aria-selected] instead of li,active

Happy to nitpick but just wanted to drop this in early :)

Copy link
Owner

@jdan jdan Apr 23, 2020

Choose a reason for hiding this comment

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

Some exit criteria could be: does this render reach-ui's Tabs without any extra effort? https://codesandbox.io/s/rough-tree-ghm6v. Multiline could still be an extra class though.

We could then reference that in the docs :) Tabs is spec-compliant (and reviewed by some strong a11y talent!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy that. I'll change these for arias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some exit criteria could be: does this render reach-ui's Tabs without any extra effort? https://codesandbox.io/s/rough-tree-ghm6v. Multiline could still be an extra class though.

We could then reference that in the docs :) Tabs is spec-compliant (and reviewed by some strong a11y talent!)

I do not know about this library. For testing i made mines in Vue like so

<menu role="tablist">
  <li role="tab" v-for="(tab, key) in tabs" :key="key" :aria-selected="key == active"  @click="active = key">
    <a href="#" v-text="tab"></a>
  </li>
</menu>

<div class="window" role="tabpanel">
  <div class="window-body">
    <!-- some random lipsum content -->
  </div>
</div>

<script>
  const app = new Vue({
    el: '#app',
    data: {
      active: 0,
      tabs: ['Desktop', 'My computer', 'Control panel', 'Devices manager', 'Hardware profiles', 'Performance']
    }
  })
</script>

This is what renders what i captured above.

@davwheat
Copy link

I don't think that it should be called multirole. Surely justified can be used on single row tabs to grow them to fit the tabs full width?

@CaptainFalcon92
Copy link
Contributor Author

I don't think that it should be called multirole. Surely justified can be used on single row tabs to grow them to fit the tabs full width?

Yes indeed.
Following the aria-x only method i do not know how one could control this however.
I understand why Microsoft went with full-width justified tabs once there are more than more row. Otherwise it looks weird.

@CaptainFalcon92
Copy link
Contributor Author

CaptainFalcon92 commented Apr 24, 2020

Hello.

Style.css has been updated. Now with [role] and [aria-selected]

  • role="tablist" for the menu tag.
  • role="tab" for the li child is not required by css* but i applied it for proper semantic.
  • role="tabpanel" on the .window element makes it behave incomplicance with tabs.
<menu role="tablist">
  <li role="tab" aria-selected="true"><a href="#">Desktop</a></li>
  <li><a href="#">My computer</a></li>
  <li><a href="#">Control panel</a></li>
  <li><a href="#">Devices manager</a></li>
  <li><a href="#">Hardware profiles</a></li>
  <li><a href="#">Performance</a></li>
</menu>
<div class="window" role="tabpanel">
  <div class="window-body">
    <!-- the tab content -->
  </div>
</div>
  • because only direct child of menu[role="tablist"] are affected, i though having li[role=tab] was not relevant. I'm not sure about the efficiency impact of attribute base css selector so less is probably more.

I still have documentation to update once we reach a satisfying state.

@CaptainFalcon92 CaptainFalcon92 changed the title [WIP] Adding tabs Adding tabs Apr 24, 2020
@jdan
Copy link
Owner

jdan commented Apr 25, 2020

Heya @CaptainFalcon92,

Great work here :) I have two bits of feedback:

  1. I'd really like this to be compatible with @reach/tabs out of the box. Meaning, the CSS introduced in this file should match up with the markup that Tabs & co. produce.

image

https://codesandbox.io/s/affectionate-merkle-evqnl?file=/src/App.js

The reason being that reach-ui has been audited extensively, and it's been my go-to for building tablists in React lately. This library is not React-specific, but that tab markup is just a byproduct.

  1. I'm a little unsure how I feel about our dependence on <menu> and .window here. Could we stick to aria-* selectors?

@jdan
Copy link
Owner

jdan commented Apr 25, 2020

I'm happy to write some docs for this btw! You've put in lots of hard work already and I appreciate it very much

Copy link
Owner

@jdan jdan left a comment

Choose a reason for hiding this comment

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

More aria Qs

@botoxparty
Copy link

botoxparty commented May 19, 2020

Hey,

So i've refactored the tabs in XP.css as you've suggested to use aria labels.

My HTML structure looks like this (removed all the parameters that don't affect styling):

<menu role="tablist">
   <button aria-selected="true">
   First Tab
   </button>
   <button aria-selected="false">
   Second Tab
   </button>
</menu>
<div role="tabpanel">
   <p>Content for the first panel</p>
</div>
<div role="tabpanel" hidden>
   <p>Content for the second panel</p>
</div>

I have gone with buttons rather than a list with links as recommended in the MDN web docs for ARIA: tab role.

Here's a link to the XP.css branch with the updated tabs:
https://github.com/botoxparty/XP.css/tree/tabs-markup-adjustment

I'd like to keep XP.css backwards compatible with 98.css, what have you decided on as your markup structure for the tabs component?

@jdan jdan closed this Jun 15, 2020
@jdan
Copy link
Owner

jdan commented Jun 19, 2020

Looks like this closed because I deleted the master branch in favor of main, sorry! Feel free to re-open continuing our discussion

@ippezshelby
Copy link

Hi @CaptainFalcon92 @jdan Can we reopen this PR and if it looks good get it merged. There is need for this and also table with fixed header and scrollable body.

@CaptainFalcon92 Could you provide us with a working fiddle

@CaptainFalcon92 CaptainFalcon92 mentioned this pull request Oct 23, 2020
@CaptainFalcon92
Copy link
Contributor Author

Hello.
It's been slightly updated and reopened at #102.

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.

5 participants