Skip to content

Conversation

@Pytal
Copy link
Contributor

@Pytal Pytal commented Feb 21, 2024

Summary

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

This reverts commit ccdd40f.

Signed-off-by: Christopher Ng <[email protected]>
@Pytal Pytal added bug Something isn't working 3. to review Waiting for reviews feature: app-navigation Related to the app-navigation component labels Feb 21, 2024
@Pytal Pytal added this to the 8.7.2 milestone Feb 21, 2024
@Pytal Pytal self-assigned this Feb 21, 2024
@Pytal
Copy link
Contributor Author

Pytal commented Feb 21, 2024

/backport to next

@emoral435
Copy link
Contributor

LGTM!

@Pytal Pytal requested a review from ShGKme February 21, 2024 18:19
Copy link
Contributor

@ShGKme ShGKme left a comment

Choose a reason for hiding this comment

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

I think this brings back an old issue and doesn't fix the new one.

List item should not be a section header. It should be assigned to the list via aria-labelledby.


There is a list of items that should not be a single list. E.g. "Active accounts" and "New group" are different things. We cannot label both by "Groups".

In this place, it should be separated into 2 lists and "Groups" should be the caption only of the second list (not level 2 heading of the page).

image


Implementation wise, I'd say, a simple solution could be to manually connect the list with NcAppNavigationCaption via aria-labelledby and a more complex and magic solution - via provide/inject label a list if it has NcAppNavigationCaption.

@susnux
Copy link
Contributor

susnux commented Feb 22, 2024

Implementation wise, I'd say, a simple solution could be to manually connect the list with NcAppNavigationCaption via aria-labelledby

So just add ul inside for the groups and add an ID to the caption + aria-labelledby to the ul?

and a more complex and magic solution - via provide/inject label a list if it has NcAppNavigationCaption.

Would this not require something like a NcAppNavigationSubList?

@Pytal
Copy link
Contributor Author

Pytal commented Feb 22, 2024

Implementation wise, I'd say, a simple solution could be to manually connect the list with NcAppNavigationCaption via aria-labelledby and a more complex and magic solution - via provide/inject label a list if it has NcAppNavigationCaption.

Will do a simple nested <ul :aria-labelledby="captionId"> then ;)

@ShGKme
Copy link
Contributor

ShGKme commented Feb 23, 2024

Would this not require something like a NcAppNavigationSubList?

But we already have a component for a list, wouldn't it work?

@ShGKme
Copy link
Contributor

ShGKme commented Feb 23, 2024

@Pytal to clarify, the header must be out of the list, it should not be a part of the list like NcAppNavigationCaption does.

<ul>
  ...first list of items...
</ul>

<span id="ID">Groups</span>
<ul aria-labelledby="ID">
  ...second list of items...
</ul>

It happens that h2 is also valid here and even better solution (without aria-labelledby), but it still have to be out of the list.

<ul>
  ...first list of items...
</ul>

<h2>Groups</h2>
<ul>
  ...second list of items...
</ul>

In any case, <NcAppNavigationCaption> being a list item must not be a header h2.

@JuliaKirschenheuter
Copy link
Contributor

@ShGKme was first 😆

@susnux
Copy link
Contributor

susnux commented Feb 23, 2024

to clarify, the header must be out of the list, it should not be a part of the list like NcAppNavigationCaption does.

Is this really needed? Because then we need to fix the whole NcAppNavigation component. Otherwise it does not work, because everything there is put inside ul.

Meaning would it not be possible to have:

<ul>
  <li><span id="ID">...</span></li> <!-- This is the caption component -->
  <li>
    <ul aria-labelledby="ID">
      <li>...</li>
    </ul>
  </li>
</ul>

@susnux
Copy link
Contributor

susnux commented Feb 23, 2024

But we already have a component for a list, wouldn't it work?

You mean NcAppNavigationItem with its default slot?
For the mentioned issue with the groups you could just use that component and make it non-collapsible.

@ShGKme
Copy link
Contributor

ShGKme commented Feb 23, 2024

Is this really needed?

Yes... We have two lists with a header in the middle (semantically and visually) that is implemented as a single list.

You mean NcAppNavigationItem with its default slot?

Sorry, I mixed up with NcAppContentList...

Because then we need to fix the whole NcAppNavigation component. Otherwise it does not work, because everything there is put inside ul.

It is only put inside ul when #list slot is used, but there is also the default slot. Cannot we pass any number of lists there? And adjust styles a bit, remove <ul> for an empty list.

Probably, here adding new NcAppNavigationList would help to reuse styles.

For the mentioned issue with the groups you could just use that component and make it non-collapsible.

I'm not sure I understand what you mean here...

@Pytal
Copy link
Contributor Author

Pytal commented Feb 23, 2024

It is only put inside ul when #list slot is used, but there is also the default slot. Cannot we pass any number of lists there? And adjust styles a bit, remove <ul> for an empty list.

Probably, here adding new NcAppNavigationList would help to reuse styles.

Right, had missed the default slot at first too. So I would say adding a new NcAppNavigationList and using it in NcAppNavigation default slot is a good solution

@Pytal
Copy link
Contributor Author

Pytal commented Feb 24, 2024

Superseded by #5302

@Pytal Pytal closed this Feb 24, 2024
auto-merge was automatically disabled February 24, 2024 01:29

Pull request was closed

@Pytal Pytal deleted the revert/5020 branch February 24, 2024 01:29
@ShGKme ShGKme modified the milestones: 8.7.2, 8.8.0 Feb 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews backport-request bug Something isn't working feature: app-navigation Related to the app-navigation component

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants