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

Feature/filter #680

Open
wants to merge 20 commits into
base: feature/templatestudies
Choose a base branch
from
Open

Feature/filter #680

wants to merge 20 commits into from

Conversation

rajuAhmed1705
Copy link
Contributor

No description provided.

Copy link

github-actions bot commented Aug 25, 2024

Visit the preview URL for this PR (updated for commit c92d500):

(expires Wed, 09 Oct 2024 11:35:25 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: 2149dad49ed83535217e50d5c18c0c8c90da629b

@ibrahimozkn ibrahimozkn self-requested a review August 27, 2024 16:20
Copy link
Contributor

@ibrahimozkn ibrahimozkn left a comment

Choose a reason for hiding this comment

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

  1. After changing any filter, if the resulting study list is empty, it becomes impossible to access the filtering options to revert the applied changes.

image

  1. Choosing both Everyone and Invite Only options in participation filter is duplicating the existing studies in the list
    image

  2. After applying a filter, if the filter results in the last remaining option, it becomes impossible to remove that filter. The user cannot revert or clear the applied filter, effectively locking them into a filtered state.
    image

@ibrahimozkn ibrahimozkn self-assigned this Sep 10, 2024
ibrahimozkn
ibrahimozkn previously approved these changes Sep 25, 2024
@ibrahimozkn ibrahimozkn dismissed their stale review September 25, 2024 09:38

requested review from other peer

@johannesvedder
Copy link
Contributor

@ibrahimozkn could you check the warnings? https://github.com/hpi-studyu/studyu/actions/runs/10976108923/job/30476497785?pr=680

@github-actions github-actions bot added the app label Oct 1, 2024
@johannesvedder
Copy link
Contributor

The e2e test seems to be broken, not sure if this is related to this PR though

@johannesvedder
Copy link
Contributor

johannesvedder commented Oct 2, 2024

Some remarks:

  • I would still show the sorting arrow next to the name and then the filter icon. Because currently you don't expect that you can still sort the items and only see the filter icon. But the filter menu does not open when you click on the name, even though the icon hover effect appears. So for now I would remove the onclick from the column name, add sorting arrow and filtering icon and have onclick actions with hover effect on both icons.
    • For long term this will probably become really messy to show two symbols next to each column. I like Supabase's approach to have a single sort and filter option on the top for all columns. This looks way cleaner:
      grafik
  • We should think about storing sorting and filtering selections in the database for each user to make these settings persistent
  • Since you can only filter by one item right now, radio buttons are a better fit than checkboxes which indicates that a user can select multiple items
  • Replace the button label "Filter" with a clearer message. "Apply filter" or just "Apply" fits better
    grafik
  • When the filter menu is shown, no other menus/popups/buttons of the page behind should be clickable. A click next to the menu should close the popup. Otherwise this leads to a weird behavior, e.g. when you click on a study while the popup is open:
    grafik
  • Whenever a filter for a column is active, this should be communicated to the user through the UI, even if the popup is closed (again look at the Supabase example where the color changes). Maybe we can change the color of the icon to the primary color whenever a filter is active for that category.

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

Successfully merging this pull request may close these issues.

3 participants