Change labels filter to dynamic height, fixed width, and shadow scroll bar

[seetohjinwei]

Summary:

Fixes #160

Changes Made:

• Change scroll-container's height to be max-height
• Change scroll-container's width to be a fixed width of 280px
• Add shadow scroll bar

Commit Message:

Change labels filter to dynamic height, fixed width, and shadow scroll

Previous, in a repository without labels, or with few labels, the
container would still be a fixed height of `400px`.

Previously, in a repository with many labels, the container would be
scrollable, but the scroll bar would be hidden.

Let's
* Change `scroll-container`'s `height` to be `max-height`
* Change `scroll-container`'s `width` to be a fixed width of `280px`
* Add shadow scroll bar

Tested on Chrome, Firefox, Safari.

[seetohjinwei]

Additionally, when using the filter input to search for specific labels, if none of the labels match the search text, there is no indication that there are no labels found.

Should we add some form of indication like No Labels Found for the above situation?

[gycgabriel]

Additionally, when using the filter input to search for specific labels, if none of the labels match the search text, there is no indication that there are no labels found.

Should we add some form of indication like No Labels Found for the above situation?

Good to have enhancement, can create a new issue.

[seetohjinwei]

Created the issue here! #164

[gycgabriel]

LGTM except for showing the scrollbar

@Eclipse-Dominator Was it preferred to hide the scrollbar for LabelFilterComponent?

I think we can look into using shadow scroll instead of scrollbar because there is a bit of jitter when the scrollbar disappears as the labels are filtered

[Eclipse-Dominator]

@Eclipse-Dominator Was it preferred to hide the scrollbar for LabelFilterComponent?

I think we can look into using shadow scroll instead of scrollbar because there is a bit of jitter when the scrollbar disappears as the labels are filtered

I think my original decision to remove the scrollbar was to preserve spacing for the popup menu as well as to keep the popup bar cleaner. I think the argument for having the scrollbar is also valid.

It might be better if we can look into using shadow scroll. I think the jitter can also be solved if the menu width size is fixed?

What do you think? @seetohjinwei

[gycgabriel]

I think we can go with shadow scroll to keep with the style. Doesn't make sense to add accessibility for only the label filters but not the issue cards.

[seetohjinwei]

Hi! Do you mean something like this: https://plainenglish.io/blog/how-to-create-horizontal-vertical-scroll-shadows?

I could try to do it

[gycgabriel]

Yes. Let's use css, not the javascript one. You can also refer to #148

[seetohjinwei]

Alright, thanks! I'll try to make these changes tonight

[seetohjinwei]

Hi! I've made the following changes:

• Add shadow scroll bar (and made the current scroll bar invisible)
• Made the panel fixed width

[Eclipse-Dominator]

LGTM other than perhaps have a similar shadow to the dashboard ver where the shadow at the edge is thinner than the center? Right now the corner looks a bit jarring

[seetohjinwei]

Changed the CSS for the shadow scroll to match the dashboard one!

[Eclipse-Dominator]

LGTM!

[gycgabriel]

One thing you could consider is having the padding inside the scrollable container, so something like this mock-up. Can also remove bottom padding altogether. Maybe also change radial gradient from 0.5 to 0.7 alpha to thicken the bottom shadow.

At the end, do remember to change your PR title and commit message to the new implementation.

[seetohjinwei]

I've removed the padding from the outer container, and re-added it to just the search input. I think the scroll-container looks a bit better without padding? I've also removed the top and bottom paddings of the entire popup menu, and changed the radial gradient value.

But, I did notice that the content is rendered above the scroll shadow, which is obvious with the coloured labels, I think #173 is related to this, so I'll try and re-implement the changes made there to this PR once that's done too.

[gycgabriel]

@seetohjinwei #173 has been merged!

[seetohjinwei]

I've made the changes with reference to 173!

[gycgabriel]

LGTM

[gycgabriel]

LGTM