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.

Sign up for GitHub

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

Jump to bottom

Admin member index: filter by member roles #17884

Merged
merged 12 commits into from
Jun 16, 2022
Merged

Admin member index: filter by member roles #17884

merged 12 commits into from
Jun 16, 2022

Conversation

aitchiss
Copy link
Contributor

@aitchiss aitchiss commented Jun 10, 2022 •
edited
Loading

What type of PR is this? (check all applicable)

  • Refactor
  • Feature
  • Bug Fix
  • Optimization
  • Documentation Update

Description

This PR updates the "member roles" section of the new filters modal to include an expandable list of member roles and checkboxes. Selecting role checkboxes and filtering should update the list of users in the UI.

Related Tickets & Documents

QA Instructions, Screenshots, Recordings

  • With the member_index_view feature flag enabled, visit /admin/member_manager/users
  • Click the filters button at the right-hand side
  • Expand the member roles section

Expand/collapse roles

  • You should see the first 6 roles and a button to see more
  • Click the button, and you should see the remaining roles
  • The button should now say 'see fewer roles'
  • Click the button again and check they collapse back down

Show/hide "Clear filter" button and status indicator

  • When no checkboxes are selected, the "Clear filter" button and the little dot next to "member roles" should not be visible
  • When any number of checkboxes are selected, the button and the little dot should be visible

Filtering behaviour

  • Select some roles and click to apply filters
  • Check the list of users matches expectations
  • Open the modal again and check the dot is visible to show filters are selected, and check the currently selected roles are in a "checked" state
CleanShot.2022-06-15.at.17.34.46.mp4

UI accessibility concerns?

  • The "see more roles" button has a static aria-label and an aria-pressed attribute, making it a 'toggle' button. Screen reader users can therefore be aware whether the list is currently expanded or collapsed
  • I also made some changes to the rendering of this filters modal so that we don't have conflicts in form ID values

Added/updated tests?

  • Yes
  • No, and this is why:
  • I need help with writing tests

[Forem core team only] How will this change be communicated?

Will this PR introduce a change that impacts Forem members or creators, the
development process, or any of our internal teams? If so, please note how you
will share this change with the people who need to know about it.

  • I've updated the Developer Docs or
    Storybook (for Crayons components)
  • This PR changes the Forem platform and our documentation needs to be
    updated. I have filled out the
    Changes Requested
    issue template so Community Success can help update the Admin Docs
    appropriately.
  • I've updated the README or added inline documentation
  • I've added an entry to
    CHANGELOG.md
  • I will share this change in a Changelog
    or in a forem.dev post
  • I will share this change internally with the appropriate teams
  • I'm not sure how best to communicate this change and need help
  • This change does not need to be communicated, and this is why not: still behind a feature flag

[optional] Are there any post deployment tasks we need to perform?

[optional] What gif best describes this PR or how it makes you feel?

coffee filter

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Jun 10, 2022
@aitchiss aitchiss force-pushed the aitchiss/filter-by-member-roles branch from 2979fc0 to 2ac6205 Compare June 10, 2022 12:04
@aitchiss aitchiss changed the title Admin member index: filter by member roles Admin member index: show member roles in filter modal Jun 10, 2022
@aitchiss aitchiss marked this pull request as ready for review June 10, 2022 13:34
@aitchiss aitchiss requested review from a team, Ridhwana and msarit and removed request for a team June 10, 2022 13:34
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Jun 10, 2022
@aitchiss aitchiss requested review from a team and removed request for a team June 10, 2022 13:34
jeremyf added a commit that referenced this pull request Jun 14, 2022
@jeremyf
Allowing UserQuery to filter on multiple roles
c7cc8c5 
I had looked at using `User.with_any_roles` but that creates one User
query per role passed.  Which is not ideal, given that we want to have
an ActiveRecord::Relation object on which we'd paginate.

This adjustment does it's best to mimic Forem's role structure and
adhear to how Rolify builds the underlying query.

An astute reader will notice that in these specs we use `role: "admin"`
and `roles: ["Admin"]`; the primary reason relates the user interface
being worked on in #17884.  It is my understanding that the
`role` approach and `roles` approach will be separated by a feature
flag (we're rolling out the `roles` approach).

Related to
- #17491
- #17884
@jeremyf jeremyf mentioned this pull request Jun 14, 2022
Merged
3 tasks
jeremyf added a commit that referenced this pull request Jun 15, 2022
@jeremyf
Allowing UserQuery to filter on multiple roles (#17919)
b4f17ed 
I had looked at using `User.with_any_roles` but that creates one User
query per role passed.  Which is not ideal, given that we want to have
an ActiveRecord::Relation object on which we'd paginate.

This adjustment does it's best to mimic Forem's role structure and
adhear to how Rolify builds the underlying query.

An astute reader will notice that in these specs we use `role: "admin"`
and `roles: ["Admin"]`; the primary reason relates the user interface
being worked on in #17884.  It is my understanding that the
`role` approach and `roles` approach will be separated by a feature
flag (we're rolling out the `roles` approach).

Related to
- #17491
- #17884
@aitchiss aitchiss marked this pull request as draft June 15, 2022 13:14
@aitchiss
Copy link
Contributor Author

Converted this back to draft as I think I can now join up with Jeremy's PR and get the whole flow working

@pr-triage pr-triage bot added PR: draft bot applied label for PR's that are a work in progress and removed PR: unreviewed bot applied label for PR's with no review labels Jun 15, 2022
@aitchiss aitchiss marked this pull request as ready for review June 15, 2022 16:49
@aitchiss aitchiss requested a review from a team as a code owner June 15, 2022 16:49
@aitchiss aitchiss requested review from a team and maestromac and removed request for a team June 15, 2022 16:49
@pr-triage pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: draft bot applied label for PR's that are a work in progress labels Jun 15, 2022
@aitchiss aitchiss changed the title Admin member index: show member roles in filter modal Admin member index: filter by member roles Jun 16, 2022
@aitchiss aitchiss mentioned this pull request Jun 16, 2022
Merged
16 tasks
jaw6
jaw6 approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@jaw6 jaw6 left a comment

Choose a reason for hiding this comment

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

This is really just me wondering... but I wonder if we could have it "show more filters" on load if any of the "more filters" were on? It's only one extra click, but the feeling I get is somehow like "it ought to remember".

@pr-triage pr-triage bot added PR: partially-approved bot applied label for PR's where a single reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Jun 16, 2022
@aitchiss
Copy link
Contributor Author

This is really just me wondering... but I wonder if we could have it "show more filters" on load if any of the "more filters" were on? It's only one extra click, but the feeling I get is somehow like "it ought to remember".

Yeah I agree @jaw6; it's especially weird if none of the "initially visible" filters are applied and you're looking at a bunch of empty checkboxes. There's a few small things I wanted to clarify with the UX/UI here anyway, so I'll add this to the list as I'm sure we can polish it up 🙂

Zhao-Andy
Zhao-Andy approved these changes Jun 16, 2022
View reviewed changes
Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

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

Lovely!

@aitchiss aitchiss merged commit b94c5de into main Jun 16, 2022
@aitchiss aitchiss deleted the aitchiss/filter-by-member-roles branch June 16, 2022 14:06
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: partially-approved bot applied label for PR's where a single reviewer approves changes labels Jun 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaw6 jaw6 jaw6 approved these changes

@Zhao-Andy Zhao-Andy Zhao-Andy approved these changes

@Ridhwana Ridhwana Awaiting requested review from Ridhwana

@msarit msarit Awaiting requested review from msarit

@maestromac maestromac Awaiting requested review from maestromac maestromac is a code owner automatically assigned from forem/platform

Assignees
No one assigned
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Member Index View Filters: Filter by "Member Roles"
3 participants
@aitchiss @jaw6 @Zhao-Andy