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

Replacing the malihu-custom-scrollbar plugin with some CSS #1175

Closed
wants to merge 4 commits into from
Closed

Replacing the malihu-custom-scrollbar plugin with some CSS #1175

wants to merge 4 commits into from

Conversation

henriyli
Copy link
Collaborator

@henriyli henriyli commented Jun 1, 2021

Doesn't look 100% the same but helps to get rid of one unmaintained plugin that's semi-useless.

@osma osma self-assigned this Jun 3, 2021
@osma osma self-requested a review June 3, 2021 14:15
@osma osma added this to the 2.12 milestone Jun 3, 2021
Copy link
Member

@osma osma left a comment

Choose a reason for hiding this comment

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

I tested this briefly and also looked at the code. Here is an illustration of how this affects the YSO front page:

image

Mostly this looks good. A few notes:

  1. The waypoint callbacks are working on the vocabulary home page and the alphabetical index (new items are loaded when you scroll down enough). But it doesn't work on the New tab that shows new concepts. You can scroll all the way down the initial list but nothing happens. This must be fixed.
  2. The scrollbar is a bit narrower than the old one, but that's OK. It's also quite a bit higher. I think this should be adjusted closer to the original height. The colors are the same as in the default Skosmos look and feel, which is OK; the above screenshot shows the Finto style, where the custom CSS should be adjusted to fix the colors (but we can do that after merging this).
  3. The commits contained quite a bit of whitespace-only changes which make the reviewing of changes a bit harder than necessary. But I think we can live with those, as this is simple enough. But for future PRs it would be better either not to reformat existing code, or if you have to, then do it in a separate commit (or even a PR) so that it's possible to ignore when reviewing.

I have no idea why we were using a static copy of resource/css/jquery.mCustomScrollbar.css instead of relying on the one under the vendor/ directory managed by Composer, but I'm glad we're getting rid of it :)

@henriyli
Copy link
Collaborator Author

henriyli commented Jun 3, 2021

  1. The new tab callback seems to be working for me. What do I need to do to reproduce the issue?

  2. I adjusted it to be a bit wider and added a box shadow to the thumb to emulate shorter height (the thumb height cannot be made shorter otherwise). How does it look now?

  3. Seems like I had auto formatting on save on in my editor but I've turned it off for now.

No idea why the CSS was a static import. Probably some brain fart from way back. :)

@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@osma
Copy link
Member

osma commented Jun 8, 2021

The new tab callback seems to be working for me. What do I need to do to reproduce the issue?

I tested this again using both Firefox 88 and Chromium on Ubuntu 20.04. I see two issues with the callback on the New tab on both browsers:

  1. If you open the A-Z tab, then click on the New tab, the UI performs a partial page load using AJAX. When you then scroll down the list of results, the callback isn't triggered at all. ISTR that the callback needs to be installed separately after the AJAX page update; probably this hasn't been done properly.
  2. If you go to the New tab directly via URL and scroll down, the callback is triggered not just once, but many times; and eventually you get a list of results which may be much older than expected. See the Network tab in this screenshot:

image

Also, after the requests have completed, there is a gap in the list of new concepts (here, "compression garment" should have been followed by other concepts from November 2020, e.g. "compression treatment", not concepts from April 2018):

image

I adjusted it to be a bit wider and added a box shadow to the thumb to emulate shorter height (the thumb height cannot be made shorter otherwise). How does it look now?

I didn't have time to look at this in very much detail but it seems to me that the height of the new scrollbar is still very different from the old one. It is also much higher on Firefox than Chromium. I'll look at this again once the callback issues have been resolved.

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.

4 participants