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

Allow toggling focus filters independently #6070

Closed
wants to merge 1 commit into from

Conversation

robertknight
Copy link
Member

This PR changes the focus filter controls to better support the existence of multiple focus filters.

On the main branch, all the active focus filters are combined into a single description, and there is one toggle button that activates / deactivates all of them. Here is a context where there is both a user filter and a page range filter active. This would represent for example a scenario where a teacher is grading a specific student in an ebook assignment where a page range is specified for the assignment:

user-page-filter-main

The fact that we have two filters but one button leads to confusing labeling when we try to reflect the state when it is on/off. Here is the button after toggling "Show all":

user-filter-main

On this branch, the two filters are represented as independent toggles (note the button presentation is not final, this PR is focused on the logical UI change):

user-page-filter

With the page range filter deactivated:

user-filter-only

Combined with a filter query:

user-filter-plus-search

The revised UI has several advantages over the previous approach:

  • The different filter queries can be toggled independently. Here for example a teacher could show annotations from other students without seeing annotations from outside the selected page range.
  • The UI better aligns with the actual data model, which tends to avoid confusion
  • It is more space efficient and there are fewer words to read
  • It avoids the UI layout changing when filters are toggled on or off, because we only change the style but not the geometry of the toggle buttons depending on their pressed / not-pressed state.

There are some further changes planned but not implemented in this PR:

  • Make it so that clearing the filter query does not affect the focus filter toggles. Currently clearing the filter query turns off all filters, which is incredibly confusing in the existing UI.
  • Show the Annotations / Notes tabs all the time, this makes the UI layout more consistent across the different filter combinations that can be active.

Base automatically changed from multiple-filter-active-states to main January 8, 2024 15:28
Instead of representing the state of multiple, logically
independent filters as a single string with one button to toggle them,
display a line of toggle buttons which each corresponds to a separate
filter (user focus, page focus, selection).

This has several advantages:

 - The user can toggle one focus filter without affecting others. For
   example in an assignment with a page range filter, a teacher can
   toggle a filter for the student they are currently grading without
   affecting the page range filter.

 - The UI better aligns with the internal data model, which avoids
   confusion

 - It is more space efficient and there are fewer words to read

 - It avoids the UI layout changing when filters are toggled on or off,
   because we just change the style but not the geometry of the toggle
   buttons depending on their pressed / not-pressed state.
@robertknight
Copy link
Member Author

Something we'll have to address with the new design is making it more obvious why nothing is being shown if nothing matches all the active filters. In this PR a blank state looks like:

blank-state-issue

We could fix this by adding some general empty-result state where the annotation list normally appears.

@robertknight
Copy link
Member Author

Closed as the UI design has changed. See #6006 (comment) for current mocks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant