Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (7de751e) 99.43% compared to head (b00d584) 99.43%.

@@           Coverage Diff           @@
##             main     #703   +/-   ##
=======================================
  Coverage   99.43%   99.43%           
=======================================
  Files         131      131           
  Lines        6502     6504    +2     
  Branches      488      488           
=======================================
+ Hits         6465     6467    +2     
  Misses         37       37           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Hey, @mvaled... I got your point here.

I'm wondering if we don't want to keep the Sidebar as aside, 'cause it is a sidebar component, and move the component mutation to the Sidebar.Items or Sidebar.ItemsGroup, 'cause this component is the one that actually can hold the navigation item.

IMHO, a Sidebar can probably handle different components that are not actually related to the main site/app navigation. WDYT?

@rluders

I'd rather have the sidebar completely as the nav, because the main purpose of (some) sidebars is the navigation. Non-links elements are almost always required; and they are not forbidden.

The first example of <nav> in MDN contains a tree of elements ol > li > a. (It even state that the links could be provide in prose 🤓 .)

On the other hand, an aside is a semantically anything (not just links and doesn't require items, groups, etc) that is indirectly related to the main content.

To be honest, I was somewhat surprised to see an aside as the default choice for a sidebar, for which the main purpose seems to be navigation (Sidebar.Item being mostly a link). But I didn't want the PR to be a breaking change.

The app I'm doing has a vertical navigation as a sidebar (like Sentry's navbar):

Maybe I could use Navbar instead, but that requires more work because of the default horizontal styling.

Well, @mvaled... it seems that you are correct. I wasn't aware of it, thanks for the explanation. So, in order to be more semantic-correct we should even avoid using the aside for the sidebar at all. Correct? So, in this case, my suggestion is to replace line#42 to use nav as default and allow people to use some other tags like div, section, or aside as they prefer.

WDYT? Does it make sense?

I found this one interesting as well: https://www.w3.org/TR/html5-author/the-aside-element.html

I'll include the 'nav' as the default. Should I also include a (soft) breaking-change in the changelog. Or do you prefer to collect changelogs at the end of the release?

@mvaled could you check the unit tests, please? :)

I think you can argue either way on this semantically quest easily. Sidebars often include navigation but aren't limited to it. Example: https://flowbite.com/react-admin-dashboard-pro/preview/

I do agree that nav is more specific, though, when navigation is all the box is used for. And I can't speak for the users of the library, but I do imagine you're right that that's usually the case.

And thank you for contributing, once the unit tests are ready we can merge :D

@rluders The tests pass now. It was the change in the aria-label, which was reverted due to the suggestion of @tulup-conner.

Thank you! Let's get it merged.