Skip to content

Conversation

@JuliaKirschenheuter
Copy link
Contributor

@JuliaKirschenheuter JuliaKirschenheuter commented Nov 13, 2023

Summary

  • Move action for opening app details to the link
  • Add alt for AppScore image
  • Remove opacity from app description because not accessible

🖼️ Screenshots

After:

Peek 2023-11-14 15-03

Peek 2023-11-14 15-02

Checklist

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

Tested and works but breaks on mobile:
image

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

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

Hi @JuliaKirschenheuter, this should be an <a /> element as suggested in the issue. For a quick fix we can just wrap the title of the app for this.

We shouldn't remove the clickability of the whole line because this is what we ultimately want to achieve.

For the Cards view, the cards should be <a /> elements themselves so we don't need to add any button.

@marcoambrosini
Copy link
Member

update - solution for having the whole row clickable:

The first <td> element in the row should have position: absolute and contain both the svg icon and the <a> element. The <a> element also has position: absolute and width: 100%
The rest is adjusting paddings, z-indexes and focus feedbacks.

This is legal HTML, it's accessible and does what we want.

Screen.Recording.2023-11-14.at.11.20.30.mov

@JuliaKirschenheuter
Copy link
Contributor Author

JuliaKirschenheuter commented Nov 14, 2023

For the Cards view, the cards should be elements themselves so we don't need to add any button.

@marcoambrosini i can't agree with you regarding Cards view. Right now we have <li class="section"> which is clickable. Then we need to create a link for icon and image.

@marcoambrosini
Copy link
Member

@JuliaKirschenheuter the link would be the whole card. Please take a look at how Airbnb deals with this in their card view

@JuliaKirschenheuter
Copy link
Contributor Author

update - solution for having the whole row clickable:

The first <td> element in the row should have position: absolute and contain both the svg icon and the <a> element.

@marcoambrosini

We have a table in that place and i can't add a link between <td> elements, that would broke a table. Please have a look in https://www.w3.org/TR/WCAG20-TECHS/H79.html

What i can do in that place: create a link over name:
Screenshot from 2023-11-14 12-00-30

@marcoambrosini
Copy link
Member

@JuliaKirschenheuter the <a> goes within the <td>

@ShGKme
Copy link
Contributor

ShGKme commented Nov 14, 2023

Agree with @marcoambrosini proposal. Making a link with position absolute over all the row makes the whole row behave like a link:

  • It has a link context menu
  • Can be opened in a new tab by mouse
  • Having a link allows using screen reader navigation to go to the next link
  • And it is the whole row, how it was before, not only text in one cell

But I'd suggest a bit different implementation, union with @JuliaKirschenheuter's suggestion.

We already have a column with the name which describes the link. We can make this text a link. And then add pseudo-element ::after to the link to stretch it as Marco wanted to do with TD. Then we won't duplicate the link text in another column and it will be positioned on on the text.

Pseudo-code:

  1. Make a name a link:

    <td class="app-name">
      <a href="link">Collaborative tags</a>
    </td>
  2. Add ::after to a link to make it stretch on the row.

    .app-name a::after {
        content: '';
        position: absolute;
        top: 0;
        left: 0;
        right: 0;
        bottom: 0;
    }
  3. Fix z-index with other buttons as Marco mentioned

  4. Remove the current click handler and replace it with the link (need to check how it works now)


One drawback. It makes it impossible to select the text on the row...

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.

Works fine in both table and grid views, opens the sidebar, works as a link, works with a screen reader and keyboard.

Name  column 2  visited  link    current page  Show details for Dashboard app

NVDA with a focus on the link:

image

@ShGKme
Copy link
Contributor

ShGKme commented Nov 14, 2023

One drawback. It makes it impossible to select the text on the row...

IMO, this is fine in this case.

@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/41310-Create_possibility_to_open_an_app_details_via_keyboard branch from 2913cd7 to 119315b Compare November 14, 2023 15:11
@AndyScherzinger AndyScherzinger added this to the Nextcloud 28 milestone Nov 14, 2023
@susnux susnux force-pushed the fix/41310-Create_possibility_to_open_an_app_details_via_keyboard branch from 119315b to 7e572f3 Compare November 14, 2023 19:02
@blizzz blizzz mentioned this pull request Nov 14, 2023
@susnux
Copy link
Contributor

susnux commented Nov 14, 2023

Cypress seems to be related needs fixing the app details tests

@blizzz blizzz mentioned this pull request Nov 14, 2023
@JuliaKirschenheuter JuliaKirschenheuter changed the title Add button for opening app details Add link for opening app details Nov 15, 2023
Add `alt` for `AppScore` image

Signed-off-by: julia.kirschenheuter <[email protected]>
@JuliaKirschenheuter JuliaKirschenheuter force-pushed the fix/41310-Create_possibility_to_open_an_app_details_via_keyboard branch from 7e572f3 to 25d8703 Compare November 15, 2023 09:50
@ShGKme ShGKme enabled auto-merge November 15, 2023 11:08
@ShGKme ShGKme merged commit af0a876 into master Nov 15, 2023
@ShGKme ShGKme deleted the fix/41310-Create_possibility_to_open_an_app_details_via_keyboard branch November 15, 2023 11:32
@ShGKme ShGKme mentioned this pull request Mar 16, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BITV]: Create possibility to open an app details via keyboard

8 participants