-
-
Notifications
You must be signed in to change notification settings - Fork 841
Pin projects filter to top of projects list mobile view #8196
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
base: gh-pages
Are you sure you want to change the base?
Pin projects filter to top of projects list mobile view #8196
Conversation
…ll function and removed pills of filters to reduce clutter on mobile view
Want to review this pull request? Take a look at this documentation for a step by step guide! From your project repository, check out a new branch and test the changes.
|
Hi @Maarimar Please do not assign reviewers to your issue, unless either you have spoken with them already or they previously reviewed the same PR and need to re-review it. In this case, one of the reviewers that you assigned is no longer active in the org. When other, available reviewers see that you have reviewers assigned already, they will likely skip over your PR and you may not have anyone reviewing your PR. I will un-assign the reviewers. |
ETA: EOD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Maarimar,
Fantastic job on your PR! Here are the things you did really well:
- You added a counter next to Clear All within the filter bar.
- The Clear All button works exactly as intended.
- You pinned the filter bar correctly to the top of the pink area.
- The scroll bar is clearly visible and functional.
- Your explanation and description of the project are beautifully written.
Here are a couple of things that caught my attention:
In the filter, as the user scrolls down to select an option, especially when reaching the bottom of the list after Status, the Cancel
and Apply
buttons disappear.
This one is a bit nitpicky: when scrolling through the project list, the filter and search bar do not fully cover the top element. A small gap remains between the filter bar and the project description. On the REI website, by comparison, the "Sort by" filter and pagination cover the entire top element while scrolling.
REI Scroll Down
Pull Request Scrolldown#1
Pull Request Scrolldown#2
Other than those critiques, everything looks great! The functionality is solid. Please re-request me once you’ve made those changes.
Thank you @dvernon5 for your feedback. I'll work on this this weekend and hopefully we can merge it soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Maarimar
Thank you for your changes!
What’s Done Well:
- The Filter and Search bar correctly stayed pinned while scrolling through projects.
- The Filter selection increments correctly, and the "Clear All" button works as expected.
- The website is fully functional.
What needs changes:
Website Changes.
- In desktop view, the Search Bar should display at the top of the project section. Right now, it’s placed below the “Search Tips” link.

- In mobile view, when the user clicks on the Filter dropdown arrow, it should display the filter contents and the Search Tips link.
In contrast, it should not include the Search Bar.
(I believe this will be resolved once the Search Bar is placed back in its original location.)
- I really like that the Filter and Search bar is pinned to the top and covers the projects while scrolling. However, in mobile view, the Filter and Search bar should span the entire width of the screen instead of leaving side gaps.
For reference, see the REI website or the Hack for LA header section:
REI Mobile View
Hack for LA Mobile View (Header Section)
- No changes are needed in desktop view (once the Search Bar is back in the original position).
Code Changes
- On
line 492
, please add spaces in your condition for readability and consistency:Instead oftotalSelected > 0
totalSelected>0
- On line 481, great job refactoring the code and implementing the ES6 reduce function. Really nice improvement!
You’re doing a fantastic job, keep going! You’re very close.
Fixes #7525
What changes did you make?
Why did you make the changes?
CodeQL Alerts
After the PR has been submitted and the resulting GitHub actions/checks have been completed, developers should check the PR for CodeQL alert annotations.
Check the PR's comments. If present on your PR, the CodeQL alert looks similar as shown
Please let us know that you have checked for CodeQL alerts. Please do not dismiss alerts.
Instructions for resolving CodeQL alerts
If CodeQL alert/annotations appear, refer to How to Resolve CodeQL alerts.
In general, CodeQL alerts should be resolved prior to PR reviews and merging
Screenshots of Proposed Changes To The Website:
Visuals before changes are applied
Visuals after changes are applied