Issue1347 malihu custom scrollbar fix #1360

Vainonen · 2022-09-13T08:14:17Z

Reasons for creating this PR

Unmaintained Malihu scrollbar plugin outdated when jQuery gets updated to version 3.6

Link to relevant issue(s), if any

Closes Upgrade/replace malihu-custom-scrollbar ahead of jQuery upgrade #1347

Description of the changes in this PR

Malihu scrollbar plugin removed and scrolling functionality done without any external libraries. This PR is partly based on @henriyli's previously closed PR of this issue: #1175.

Known problems or uncertainties in this PR

When list of new and deprecated concepts is scrolled and at least one API request made, clicking the tab "new and deprecated" results to an empty list
Scroll bar color is changed as mentioned in the previous PR of this issue: Replacing the malihu-custom-scrollbar plugin with some CSS #1175.
tab header anchor should be made inactive if tab listing is shown
addresses in browser address bar do not always change when changing between sidebar tabs (e. g. if in "new and deprecated" tab, clicking "hierarchy" tab leaves address in browser unchanged)
refactoring of code and renaming of variables more conviniently needed

Checklist

I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

codecov · 2022-09-13T08:19:10Z

Codecov Report

Base: 70.68% // Head: 70.68% // No change to project coverage 👍

Coverage data is based on head (e350661) compared to base (2326cee).
Patch has no changes to coverable lines.

❗ Current head e350661 differs from pull request most recent head ee9068e. Consider uploading reports for the commit ee9068e to get more accurate results

Additional details and impacted files

@@                  Coverage Diff                  @@
##             upgrade-jquery-3.6    #1360   +/-   ##
=====================================================
  Coverage                 70.68%   70.68%           
  Complexity                 1646     1646           
=====================================================
  Files                        32       32           
  Lines                      3786     3786           
=====================================================
  Hits                       2676     2676           
  Misses                     1110     1110

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kinow

Haven't tested it but looked at the code from GH UI and it's looking really good!!! One minor comment.

kinow · 2022-09-13T08:22:01Z

resource/css/styles.css

+  --scrollbarWidth: 14px;
+  --scrollbarBg: #b9c1c6;
+  --scrollbarThumb: #474b4f;
+}


This file already has a :root block elsewhere with colors, font sizes, etc. Maybe worth keeping the root variables all in a single place.

Yep. And the variable names should be brought in line with the naming and conventions of the other css variables. To nudge this in the right direction, the beginning of the :root element in styles.css could look like this, with the corresponding changes for the variable names in styles.css row 2237 onwards:

:root { /* Color definitions */ --dark-color: #394554; /* topbar nav link text, dropdown text, buttons */ --secondary-dark-color: #3C535B; /* disabled buttons, bg of "content language" dropdown label in header */ --medium-color: #00748F; /* used for skosmos service name (top left logo), link texts (a), header bar, hovered dropdown menu items, placeholders in form input fields */ --light-color: #D4EDEB; /* used for concept property dividers */ --link-hover-color: #23527C; /* used for hovered links, clipboard icon */ --tooltip-bg-color: #1E1E1E; --tooltip-text-color: #00ACD3; --tooltip-border-color: #75889F; /*also used as typeahead focus border */ --main-bg-color: #D5DBDE; --alert-color-pale: #D95F8A; --alert-color-bright: #ED0D6C; --uri-id-text-color: #006621; /* only used in search results */ --scroll-box-color: #474b4f; /* used for the sidebar scroll */ --scroll-bar-color: #b9c1c6; /* used for the sidebar scroll */ --scroll-bar-width: 14px; /* used for the sidebar scroll */

Vainonen · 2022-09-16T08:42:52Z

A new question/issue #1361 made after commit Sidebar active nav tab made unclickable. Making current tab inactive produces a feature that the previous tab is inactive as long as next tab content is loading. Is this feature bearable or a bug to be fixed?

joelit

I made a couple of suggestions for tweaking the CSS jand JS
The bigger issue seems to be that the nav tabs of the sidebar are now triggering full page loads instead of partial ones
- Also, the anchor of the hierarchy tab points to the Skosmos front page, omitting the vocabulary ID. It's not a big thing to change - but first and foremost the page load triggered by the nav tab change should be a partial one.
- This might also cause the problem with choosing another tab while the sidebar content is loading resulting in an empty concept info like this:

joelit · 2022-09-16T09:30:31Z

resource/css/styles.css

+  --scrollbarWidth: 14px;
+  --scrollbarBg: #b9c1c6;
+  --scrollbarThumb: #474b4f;
+}


Yep. And the variable names should be brought in line with the naming and conventions of the other css variables. To nudge this in the right direction, the beginning of the :root element in styles.css could look like this, with the corresponding changes for the variable names in styles.css row 2237 onwards:

:root { /* Color definitions */ --dark-color: #394554; /* topbar nav link text, dropdown text, buttons */ --secondary-dark-color: #3C535B; /* disabled buttons, bg of "content language" dropdown label in header */ --medium-color: #00748F; /* used for skosmos service name (top left logo), link texts (a), header bar, hovered dropdown menu items, placeholders in form input fields */ --light-color: #D4EDEB; /* used for concept property dividers */ --link-hover-color: #23527C; /* used for hovered links, clipboard icon */ --tooltip-bg-color: #1E1E1E; --tooltip-text-color: #00ACD3; --tooltip-border-color: #75889F; /*also used as typeahead focus border */ --main-bg-color: #D5DBDE; --alert-color-pale: #D95F8A; --alert-color-bright: #ED0D6C; --uri-id-text-color: #006621; /* only used in search results */ --scroll-box-color: #474b4f; /* used for the sidebar scroll */ --scroll-bar-color: #b9c1c6; /* used for the sidebar scroll */ --scroll-bar-width: 14px; /* used for the sidebar scroll */

resource/js/docready.js

joelit

Apparently the clunkier JS was working better so better leave it like it was. Also, the asynchronous page loading was broken due to mismatching jQuery version.

sonarcloud · 2022-09-16T12:24:58Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
0.0% Duplication

Vainonen · 2022-09-16T12:39:02Z

Thank you @joelit & @kinow for reviewing and comments!

Vainonen added 4 commits September 7, 2022 15:40

commit copied from henriyli:sidebar

87e74f2

Prevents scrolling launching multiple callbacks

e93f765

Fixed alphabetical search callbacks

449fdf9

Fixed multiple callbacks for new concepts list

dc4d7a4

kinow reviewed Sep 13, 2022

View reviewed changes

Sidebar active nav tab made unclickable

5099078

miguelvaara assigned miguelvaara and Vainonen Sep 13, 2022

Vainonen changed the base branch from master to upgrade-jquery-3.6 September 16, 2022 07:26

joelit added this to the 2.16 milestone Sep 16, 2022

Vainonen mentioned this pull request Sep 16, 2022

Clicking previous sidebar nav tab is not possible while a new sidebar content is loading #1361

Open

joelit self-requested a review September 16, 2022 09:12

joelit requested changes Sep 16, 2022

View reviewed changes

Improved CSS naming

e350661

joelit approved these changes Sep 16, 2022

View reviewed changes

Added return values to callback functions

ee9068e

joelit marked this pull request as ready for review September 16, 2022 12:25

Vainonen merged commit bb730b6 into upgrade-jquery-3.6 Sep 16, 2022

Vainonen deleted the issue1347-malihu-custom-scrollbar-fix branch September 16, 2022 12:36

Vainonen added the maintenance Dependency changes, security updates, infrastructure tweaks & general mainenance label Sep 16, 2022

osma mentioned this pull request Sep 16, 2022

Upgrade to jQuery 3.6 #1352

Merged

4 tasks

Vainonen mentioned this pull request Sep 20, 2022

Malihu custom scrollbar should be replaced NatLibFi/Skosmos-widget-wiki#10

Closed

osma mentioned this pull request Oct 4, 2022

fix auto-scrolling and highlighting current concept in hierarchy #1372

Merged

4 tasks

osma mentioned this pull request Oct 6, 2022

scroll hierarchy to selected concept instead of children #1374

Merged

4 tasks

Vainonen mentioned this pull request Oct 6, 2022

Upgrade/replace malihu-custom-scrollbar ahead of jQuery upgrade #1347

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue1347 malihu custom scrollbar fix #1360

Issue1347 malihu custom scrollbar fix #1360

Vainonen commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading

kinow left a comment

kinow Sep 13, 2022

joelit Sep 16, 2022

Vainonen commented Sep 16, 2022

joelit left a comment

joelit Sep 16, 2022

joelit left a comment

sonarcloud bot commented Sep 16, 2022

Vainonen commented Sep 16, 2022

Issue1347 malihu custom scrollbar fix #1360

Issue1347 malihu custom scrollbar fix #1360

Conversation

Vainonen commented Sep 13, 2022 • edited Loading

Reasons for creating this PR

Link to relevant issue(s), if any

Description of the changes in this PR

Known problems or uncertainties in this PR

Checklist

codecov bot commented Sep 13, 2022 • edited Loading

Codecov Report

kinow left a comment

Choose a reason for hiding this comment

kinow Sep 13, 2022

Choose a reason for hiding this comment

joelit Sep 16, 2022

Choose a reason for hiding this comment

Vainonen commented Sep 16, 2022

joelit left a comment

Choose a reason for hiding this comment

joelit Sep 16, 2022

Choose a reason for hiding this comment

joelit left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 16, 2022

Vainonen commented Sep 16, 2022

Vainonen commented Sep 13, 2022 •

edited

Loading

codecov bot commented Sep 13, 2022 •

edited

Loading