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

Filters: Add prefix label to the single query filters #2804

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

ryelle
Copy link
Contributor

@ryelle ryelle commented Jul 31, 2024

Fixes #2784 - this prefixes the single select filters with the filter name, to give it a more accessible name.

  • Level filter is prefixed with "Level: ", ex "Level: Advanced"
  • Content type filter is prefixed with "Type: ", ex "Type: Course"
  • Completion status is prefixed with "Status: ", ex "Status: Completed"

cc @ndiego @kathrynwp Do those prefixes make sense? Using the full title for the last two would be too long, IMO, so I went with a short version.

Screenshots:

Learning pathway

Before After
Screen Shot 2024-07-31 at 13 48 08 Screen Shot 2024-07-31 at 13 42 41

Lesson archive (with "Advanced" filter)

Before After
Screen Shot 2024-07-31 at 13 48 26 Screen Shot 2024-07-31 at 13 43 48

Search results

Before After
Screen Shot 2024-07-31 at 13 48 45 Screen Shot 2024-07-31 at 13 45 02

My courses

Before After
Screen Shot 2024-07-31 at 13 52 41 Screen Shot 2024-07-31 at 13 52 54

@ryelle ryelle added the Accessibility Fix or enhancement related to accessibility. label Jul 31, 2024
@ryelle ryelle requested a review from renintw July 31, 2024 17:54
@ryelle ryelle self-assigned this Jul 31, 2024
@ndiego
Copy link
Member

ndiego commented Jul 31, 2024

Visually, it looks great. Thanks @ryelle 🚢

@renintw
Copy link
Contributor

renintw commented Jul 31, 2024

Ah, I was also working on the same stuff but a different ticket.

This is the suggested label update mentioned there:

I recommend we update these labels to the following for clarity:

All --> Skill Level
Type --> Content Type

Option 1 - Fix the label

Content Type Skill Level
Screenshot 2024-08-01 at 02 33 52 Screenshot 2024-08-01 at 02 34 48

Option 2 - Same as this PR

Content Type Skill Level
Screenshot 2024-08-01 at 02 47 31 Screenshot 2024-08-01 at 02 52 03

Option 2 looks better to me.

Q: Do we consider removing the label on the dropdown? Maybe it's fine for now and can leave it to post-launch.
Markup on 2024-08-01 at 02:34:16

@@ -534,7 +534,7 @@ function get_student_course_options( $options ) {
$label = $options[ $selected_slug ] ?? $options['all'];

return array(
'label' => $label,
'label' => sprintf( __( 'Status: %s', 'wporg-learn' ), $label ),
'title' => __( 'Completion status', 'wporg-learn' ),
Copy link
Contributor

@renintw renintw Jul 31, 2024

Choose a reason for hiding this comment

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

Can you change the first letter of "status" to uppercase to make it consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use sentence case across the site, so leaving "status" lowercase in "Completion status" is correct. "Content Type" and "Skill Level" should probably be updated to "Content type" and "Skill level" instead. Unless I'm using that rule too broadly — maybe a check from @thetinyl would help.

Choose a reason for hiding this comment

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

Yes, let's use sentence case. Thanks for checking!

@ryelle
Copy link
Contributor Author

ryelle commented Jul 31, 2024

Ah sorry @renintw, I wasn't sure what that ticket was referencing, now I understand. If we went with Option 1, there is no feedback about which item is currently selected, and that info is not exposed anywhere else. So I also think Option 2 is better.

Q: Do we consider removing the label on the dropdown? Maybe it's fine for now and can leave it to post-launch.

That's a wider question, as the change would happen in the query-filter block itself. I would prefer to decouple that change from Learn. I'm making a companion issue in wporg-mu-plugins right now to track updating this on other sites, I'll add a note about this there.

@renintw
Copy link
Contributor

renintw commented Jul 31, 2024

Ah sorry @renintw, I wasn't sure what that ticket was referencing, now I understand. If we went with Option 1, there is no feedback about which item is currently selected, and that info is not exposed anywhere else. So I also think Option 2 is better.

Not at all 🙂 Yeah, that's also the reason I prefer Option 2.

Q: Do we consider removing the label on the dropdown? Maybe it's fine for now and can leave it to post-launch.

That's a wider question, as the change would happen in the query-filter block itself. I would prefer to decouple that change > from Learn. I'm making a companion issue in wporg-mu-plugins right now to track updating this on other sites, I'll add a note about this there.

I just looked around a bit at how other sites handle this and found an approach that might be good- adding an icon as a prefix to the button label, similar to how we switch GH branches here. The title inside the dropdown doesn't need to be removed in this way.

@ryelle
Copy link
Contributor Author

ryelle commented Jul 31, 2024

I just looked around a bit at how other sites handle this and found an approach that might be good- adding an icon as a prefix to the button label, similar to how we switch GH branches here. The title inside the dropdown doesn't need to be removed in this way.

Hm, I don't know what icons that would understandably communicate "level" and "type", and icons instead of text can also be an accessibility issue.

I've added a note here to address whether the dropdown title should be hidden: WordPress/wporg-mu-plugins#645

@kathrynwp
Copy link
Collaborator

Do those prefixes make sense? Using the full title for the last two would be too long, IMO, so I went with a short version.

@ryelle Yes these work for me!

@renintw
Copy link
Contributor

renintw commented Jul 31, 2024

Hm, I don't know what icons that would understandably communicate "level" and "type", and icons instead of text can also be an accessibility issue.

This might need some feedback from the design team. I was thinking that using an appropriate icon along with text inside the dropdown is also likely to help users get familiar with the filter quickly. And I think the icon can have an aria-label or aria-labelledby to handle the accessibility issue?

This isn't a blocker or something that must be done, just thought it might be worth having a preliminary discussion of the possibilities.

I've added a note here to address whether the dropdown title should be hidden: WordPress/wporg-mu-plugins#645

Thanks!

@ndiego ndiego linked an issue Aug 1, 2024 that may be closed by this pull request
@ryelle
Copy link
Contributor Author

ryelle commented Aug 1, 2024

This might need some feedback from the design team. I was thinking that using an appropriate icon along with text inside the dropdown is also likely to help users get familiar with the filter quickly. And I think the icon can have an aria-label or aria-labelledby to handle the accessibility issue?

The accessibility issue is more about understanding what the button does, not for screen reader users. For example, a sighted user might see an icon and without knowing what it means, might not click on it. This article has a good overview of accessible icon design & use - "An icon’s design should communicate its function clearly and intuitively, reducing the cognitive load on users." I don't know that there is something universally recognizable for these filters.

The github button works a) because we're technical users, and we've seen that icon many times, and b) it's labelled immediately next to the button:

Screenshot 2024-08-01 at 1 14 59 PM

From WordPress/wporg-mu-plugins#645, it sounds like this approach + removing the title from the dropdown works, so I'm going to merge this. We can always iterate on it more.

Copy link
Contributor

@renintw renintw left a comment

Choose a reason for hiding this comment

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

LGTM 👍 And thanks for sharing that a11y article!

Have we decided to go with the short version? Maybe we should tag Destiny to let her know about the change to her proposal.

@ryelle ryelle force-pushed the update/query-filter-labels branch from c554ece to c1bf3b0 Compare August 1, 2024 18:32
@ryelle
Copy link
Contributor Author

ryelle commented Aug 1, 2024

Have we decided to go with the short version? Maybe we should tag Destiny to let her know about the change to her proposal.

I think so, in the interest of space. I'll comment on the other issue so she can reopen it if that exact text was required.

return $level->slug === $selected_slug;
}
);
$selected_level = wp_list_filter( $levels, array( 'slug' => $selected_slug ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@ryelle ryelle merged commit 64846d6 into trunk Aug 1, 2024
1 check passed
@ryelle ryelle deleted the update/query-filter-labels branch August 1, 2024 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility Fix or enhancement related to accessibility.
Projects
Status: Done
5 participants