Responsive scrollable filter bar #382

JiaXinEu · 2024-07-14T14:15:14Z

Summary:

Fixes #377

Type of change:

✨ Enhancement

Changes Made:

Add BreakpointObserver to observe and respond to screen size changes
Add private method to update styling of filter-bar
Put dropdown-filters in scrollable container

Screenshots:

Proposed Commit Message:

Modify responsiveness of filters to screen size change

The filter bar is cut off when screen width is reduced. We should 
make the design responsive.

Let's make filter bar scrollable when it cannot fit on screen.

Checklist:

I have tested my changes thoroughly.
I have created tests for any new code files created in this PR or provided a link to a issue/PR that addresses this.
I have added or modified code comments to improve code readability where necessary.
I have updated the project's documentation as necessary.

Arif-Khalid

I see the vision and am open to the possibility of using a scrollable filters container like you have implemented.
However, I feel that the scrollbar is not very intuitive to the user, i.e., not what you would find on most other sites. It will also be quite hard to use on a small screen like a mobile screen compared to a hamburger or dropdown menu, which is the very audience this change is supposed to cater to.
I would prefer a different approach but you are free to continue working on this approach and try to apply some cosideration for mobile users who don't have that much space to scroll horizontally for example left and right scroll buttons at the ends of the scrollbar.

Arif-Khalid · 2024-07-23T07:07:54Z

src/app/shared/filter-bar/filter-bar.component.ts

+
+    this.breakpointSubscription = this.breakpointObserver.observe([Breakpoints.Small, Breakpoints.XSmall]).subscribe((result) => {
+      this.isSmallScreen = result.matches;
+      this.updateSpan();
+    });
+
+    this.breakpointSubscription = this.breakpointObserver.observe([Breakpoints.Medium]).subscribe((result) => {
+      this.isMediumScreen = result.matches;
+      this.updateSpan();
+    });


While this works, would it be better to take advantage of the auto scaling capabilities of css?
We can avoid these breakpoints and corresponding values since the parent colspan of 7 scales according to screen size already, such that the children widths are always relative to the parent without changing their individual colspan or the parents.
In order to preserve the full child item being shown when the screen shrinks, consider instead using min-width/max-width css properties with media queries to shift responsiveness logic to css where possible.

Arif-Khalid · 2024-07-23T07:14:52Z

src/app/shared/filter-bar/filter-bar.component.css

+  overflow-x: auto;
+  display: flex;
+  justify-content: center;
+  scrollbar-width: 5px;


Should this be using the firefox specific styling?
It would be better to use webkit:scrollbars pseudo-class selector in addition to this to properly style scrollbars in all browsers.
See https://stackoverflow.com/questions/15420314/how-to-make-custom-scrollbars-show-in-all-browsers

Arif-Khalid · 2024-07-23T07:15:51Z

src/app/shared/filter-bar/filter-bar.component.css

+  margin-right: 8px;
+  overflow-x: auto;
+  display: flex;
+  justify-content: center;


Is there a reason for justify-content and changing it when the screen is smaller? There seems to be no difference if I remove justify content in both classes in testing.

JiaXinEu · 2024-08-01T16:39:03Z

I see the vision and am open to the possibility of using a scrollable filters container like you have implemented. However, I feel that the scrollbar is not very intuitive to the user, i.e., not what you would find on most other sites. It will also be quite hard to use on a small screen like a mobile screen compared to a hamburger or dropdown menu, which is the very audience this change is supposed to cater to. I would prefer a different approach but you are free to continue working on this approach and try to apply some cosideration for mobile users who don't have that much space to scroll horizontally for example left and right scroll buttons at the ends of the scrollbar.

Thank you for a better insight of the issue. The current PR uses a togglable menu approach

Arif-Khalid

Great implementation of a hamburger dropdown and I like the substitution of mat grid you made.
However, there is some redundancy and minor errors in regards to CSS to fix.
Additionally, would it be better to put the app-label-filters into the hamburger dropdown as well since on small screens at first glance it now seems to suggest it denotes something about all filters.

Arif-Khalid · 2024-08-12T03:28:56Z

src/app/shared/filter-bar/filter-bar.component.css

+      display: block;
+      position: absolute;
+      width: 100%;
+      top: calc(20% + 8px); /* 8px margin from ul */


Would it be better to use a calculation relative to the app-filter-bar? By applying a position: relative to the app filter bar you can position the child element of class filters relative to that parent when the child has position: absolute. Therefore you could then do something like top: 100%; instead on this element and it will be positioned nicely under the filter bar.

Could you share your reasoning behind the 8px margin from ul as commented? I find it uneccessary and it would be better to associate the expansion more closely with the app-filter-bar.

Arif-Khalid · 2024-08-12T03:37:37Z

src/app/shared/filter-bar/filter-bar.component.css

+ul {
+  margin: 8px;
+  padding: 0;
+  display: flex;
+  align-items: center;
+  justify-content: space-between;
+  list-style-type: none;


Good substitution for the mat-grid components. Could you enforce a height of 80px so that no vertical scrollbar appears ? This was was enforced by the previous rowHeight property.

Arif-Khalid · 2024-08-12T03:39:47Z

src/app/shared/filter-bar/filter-bar.component.css

 }

-.dropdown-filters {
+.search-bar {


Is this class necessary?

Arif-Khalid · 2024-08-12T03:42:44Z

src/app/shared/filter-bar/filter-bar.component.css

+  display: none;
+  cursor: pointer;
+
+  @media (max-width: 1148px) {


At about 1200-1300 px range there is overlapping of components

Would it be better to use a more standard, larger breakpoint of 1400px?
Refer to https://getbootstrap.com/docs/5.0/layout/breakpoints/ for some standard breakpoints.

Arif-Khalid · 2024-08-12T03:42:54Z

src/app/shared/filter-bar/filter-bar.component.css

+  display: flex;
+  align-items: center;
+
+  @media (max-width: 1148px) {


Same comment about breakpoints

Arif-Khalid · 2024-08-12T03:46:14Z

src/app/shared/filter-bar/filter-bar.component.css

+        display: inline-block;
+      }
+    }
+  }
+
+  .filter-item {
+    padding: 0 10px;
+    display: block;


Is there a need for display: block and display: inline-block? Children of flexbox are automatically given display: inline-block

JiaXinEu · 2024-09-30T04:49:47Z

Additionally, would it be better to put the app-label-filters into the hamburger dropdown as well since on small screens at first glance it now seems to suggest it denotes something about all filters.

Thank you for the feedback. I have moved label-filter into the hamburger dropdown as well, and done the minor fixes on css as suggested.

Arif-Khalid

Your changes make the app a lot more usable on smaller devices. Thanks so much for your contributions!
LGTM!

JiaXinEu added 4 commits July 13, 2024 17:05

Modify filter bar to screen size change

29b4164

Modify responsiveness of filter to medium size screen

5b3a008

Update code comment

b537e4e

Remove redundant lines

af8102e

Arif-Khalid requested changes Jul 23, 2024

View reviewed changes

Update responsive toggle menu

4c8c04b

Arif-Khalid requested changes Aug 12, 2024

View reviewed changes

JiaXinEu added 2 commits September 29, 2024 14:16

Move label-filter into hamburger dropdown

7d6ecfc

Merge master into branch

107638c

Remove redundant styling

ecd8ff3

Arif-Khalid approved these changes Oct 25, 2024

View reviewed changes

Arif-Khalid merged commit c17c4d7 into CATcher-org:main Oct 25, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Responsive scrollable filter bar #382

Responsive scrollable filter bar #382

JiaXinEu commented Jul 14, 2024

Arif-Khalid left a comment

Arif-Khalid Jul 23, 2024

Arif-Khalid Jul 23, 2024

Arif-Khalid Jul 23, 2024

JiaXinEu commented Aug 1, 2024

Arif-Khalid left a comment

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

Arif-Khalid Aug 12, 2024

JiaXinEu commented Sep 30, 2024

Arif-Khalid left a comment

Responsive scrollable filter bar #382

Responsive scrollable filter bar #382

Conversation

JiaXinEu commented Jul 14, 2024

Summary:

Type of change:

Changes Made:

Screenshots:

Proposed Commit Message:

Checklist:

Arif-Khalid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaXinEu commented Aug 1, 2024

Arif-Khalid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaXinEu commented Sep 30, 2024

Arif-Khalid left a comment

Choose a reason for hiding this comment