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

Add bootstrap5 theme #3222

Merged
merged 133 commits into from
May 22, 2024
Merged

Add bootstrap5 theme #3222

merged 133 commits into from
May 22, 2024

Conversation

EreMaijala
Copy link
Contributor

@EreMaijala EreMaijala commented Nov 15, 2023

This is initial work for adding a bootstrap5 theme that uses Bootstrap 5.x. and to include enough back-compatibility support to make it possible to inherit other themes from it. Still pretty much work in progress but comments welcome.

Principles:

  • Keep template changes from bootstrap3 to minimum, and only do string replaces
  • Add bs3-compat.scss and bs3-compat.js as a compatibility layer

Here are the most significant template changes:

data-toggle="collapse" => data-bs-toggle="collapse"
data-toggle="dropdown" => data-bs-toggle="dropdown"
data-target => data-bs-target
data-dismiss="modal" => data-bs-dismiss="modal"

Other things worth noting:

  • visually-hidden replaces the sr-only class, but the latter is still available via bs3-compat.scss
  • font-awesome has been updated ($fa-font-path has changed too).
  • popover, tooltip, collapse and modal are different, so the following files have differences between bootstrap3 and bootstrap5:
    • account_ajax.js
    • cart.js
    • channels.js
    • facets.js
    • lightbox.js
  • Where bootstrap3 uses the in class, bootstrap5 uses open
  • legend does not have huge bottom margin in bootstrap5
  • dark theme does not work properly at the moment
  • header is a bit different and needs manual adjustment from bootstrap3
  • Tabs now have the active class on the a element instead of li (bs3-compat.js handles some of this, but style changes may be required as well).
  • npm run build:scssonly can be used to compile scss for scss-only themes (bootstrap5, sandal5). Also npm run build:css does that.
  • compiling scss requires npm install to be run in the theme directory first

TODO:

  • Commit templates before merging.
  • Do we want to clean up LESS/SCSS conditionals from scss files or keep them for parity with bootstrap3/sandal?
  • Ensure that there are no conflicts with Bootstrap 5 and VuFind themes regarding accessibility (Wave and/or Axe could be useful tools)
  • Document changes required to templates
  • Document changes required with less to sass transition
  • Update channels.js with latest changes from bootstrap3
  • Enable check-bootstrap5 in QA before merging.
  • When merging, add changelog note about theme transition plan (bootstrap5 = beta in 10.0; bootstrap3 will be deprecated in 10.1 and bootstrap5/sandal5 promoted to default; bootstrap3 and its children will be removed in 11.0)

TODO (Issues):

  • White on white text - VuFind link at top left and theme/language dropdowns appear as white-on-white (Bootstrap5 theme only).
    • Should be fixed.
  • Similar Items - formatted incorrectly in sidebar and record tabs (Bootstrap5 and Sandal5).
    • Should be fixed.
  • Field selection dropdown in search box - out of alignment (Bootstrap5 and Sandal5).
    • Should be fixed.
  • Print Record - formatting is strange (Bootstrap5 and Sandal5).
    • Should be fixed.
    • I'm still seeing the issue in both Chrome and FF, with cleared caches - ST
    • Oops, I was looking at the actual print results, fixed again. -EM
    • Big improvement! Remaining nitpick: I've posted an updated screenshot below showing that text color still varies in print preview (everything is monochrome in legacy themes) so the last thing to possibly fix is text color. - ST
    • Should be fixed as well now (BS5 makes color utils !important by default, had to disable that). -EM
  • Format and availability lozenges - font too small (Sandal5, when compared with Sandal).
    • Should be fixed. -EM
  • Floating "back" - extra link text at top of filter columns (Bootstrap5 and Sandal5).
    • Should be fixed (was a problem in dev branch, see Fix visibility of off-canvas toggles. #3573).
    • I'm still seeing the issue in both Chrome and FF, with cleared caches - ST
    • Missed a file in commit, should be properly fixed now. -EM
  • Main VuFind logo - The main VuFind logo needs more emphasis and possibly the registered trademark symbol (Bootstrap5 needs bold/etc., both Bootstraps lack the registered trademark symbol)
    • Underlining and a bit more emphasis added. -EM
  • Print preview for multi-selected items in Favorites list - Print view for selected items in Favorites lists contains graphics that don't render correctly. (Bootstrap5 and Sandal5)
    • Should be fixed now along with the other print issues. -EM
  • Record tabs ghost highlighting - Previously selected tab still looks selected (but with slightly different styling than the actually selected tab). (Bootstrap5 and Sandal5)
    • Should be fixed. -EM
  • Contrast in form element borders - Borders outside search boxes and other form elements are not contrasty enough. (Sandal5, and also Bootstrap5)
    • Focus outline for sandal5 fixed, but I can't see the other issues. -EM
    • I see a small difference in the two bootstrap versions, but I think the bootstrap5 version is slightly MORE contrasty. No change needed (unless someone who knows accessibility best practices better says differently.)
  • Hover behavior in header - Links in header need updates to styling in new themes (Bootstrap5 especially; also Sandal5)
    • Should be much better now. However, the hover highlight is not quite the same between bootstrap3 and bootstrap5, and I'm not sure it should be either. Now everything has the same hover effect. -EM
  • Advanced search limit options - Height of this element needs to be increased. (Bootstrap5 and Sandal5)
    • Fixed. -EM
    • Yes, you fixed those three... but I just noticed that in New Items, there's a similar element for Department that needs to be expanded in the same way in the new themes.
    • Now fixed as well. -EM
  • Links in Channels don't work - Links in the pop-out item menus don't work when clicked. (Bootstrap5 and Sandal5)
    • Fixed. -EM
  • Problematic click behavior in Your Account menus - Clicking a menu item results in illegible black boxes (Bootstrap5, in contrast with Bootstrap3)
    • Fixed. -EM
  • Mouseover is black in Browse and Alphabrowse - Hovered-over menu items are illegible due to black highlighting; in legacy version, highlighting is gray. (Bootstrap5)
    • Fixed. I agree that the highlight color is odd, but it's equally odd in old sandal. -EM
    • I'm still seeing hovered-over menu items as having black highlighting in bootstrap5 - even on a browser I've never used before. Can you check?
    • Had to tweak nav-link colors and make the navbar ones a special case instead of the other way round. -EM
  • Narrow dropdown on System Maintenance page in Admin area - The dropdown box in the last row on this page is not wide enough to display the word "standard" in any of the themes.
    • Fixed. The width is now relative to the font size, and padding has been reduced to allow for more text in select fields in general. -EM
    • On recheck, I see that the entire word "standard" is now visible in bootprint3 and in sandal5. However, the final 'd' is cut in half in the two bootstraps, and in sandal "standard" ends in the middle of the 'r.' So, a little more tweaking? -ST
    • Tweaked more for bootstrap5 and sandal5. I'm avoiding touching the old themes here. -EM
    • The word "standard" is fully visible now in bootstrap5 and sandal5! This is done. :)
  • Extra X in search box while typing - There's an extra X to clear your search in the searchbox. (Bootstrap5 and Sandal5)
    • Should be fixed now. -EM
    • Confirmed - fixed! - ST
  • Facets filtering lightbox layout - This lightbox has issues in the new themes - see below. (Bootstrap5 and Sandal5)
    • Should be fixed now. -EM
    • Confirmed - fixed! - ST
  • Responsive table for search history - The enhancements from PR 3204 are not fully implemented in the new themes. (Bootstrap5 and Sandal5)
    • Should be fixed now. -EM
    • Confirmed - fixed! - ST

@demiankatz demiankatz added this to the 10.0 milestone Nov 15, 2023
@demiankatz demiankatz added new feature accessibility architecture pull requests that involve significant refactoring / architectural changes labels Nov 15, 2023
@crhallberg
Copy link
Contributor

I wonder if we can reduce the template changes even further by re-mapping the data attributes with bs3-compat.js.

const dropdownElementList = document.querySelectorAll('[data-dropdown]');
dropdownElementList.forEach((dropdownToggleEl) => {
   const target = dropdownToggleEl.getAttribute("data-target");
   if (target) {
       dropdownToggleEl.setAttribute("data-bs-target", target)
   }
});

const dropdownList = [...dropdownElementList].map(
    (dropdownToggleEl) => new bootstrap.Dropdown(dropdownToggleEl)
);

For the templates that aren't changed from bs3, do we want to inherit those to make it clear which templates we need to update when we make changes?

@demiankatz demiankatz mentioned this pull request Nov 15, 2023
4 tasks
@EreMaijala
Copy link
Contributor Author

I wonder if we can reduce the template changes even further by re-mapping the data attributes with bs3-compat.js.

I haven't actually tried it yet, but I suspect we'd have trouble with the initialization order of the scripts. We could make bs3-compat run early, but then it could miss something. We'd also have to employ mutation observers for any dynamically loaded content. But I have a couple of other ideas for runtime patching..

For the templates that aren't changed from bs3, do we want to inherit those to make it clear which templates we need to update when we make changes?

My aim currently is to make it so that you could run a script to update bootstrap5 from bootstrap3. This would allow the theme to be completely standalone to avoid any confusion. But inheriting bootstrap3 is certainly an option, and I'm not committed to any specific approach yet. :)

@EreMaijala
Copy link
Contributor Author

Regardless of the approach, we could of course leave all template files out of git if the script works. That'd make this PR quite a bit smaller.

@demiankatz
Copy link
Member

Regardless of the approach, we could of course leave all template files out of git if the script works. That'd make this PR quite a bit smaller.

That would definitely make review easier, though of course we'll want to commit the files in the end. (If we're able to go with a script-based approach, we should also add a CI test similar to lessToSass to ensure nobody forgets to keep the files in sync). In any case, I definitely like the fool-proof nature of the script-based approach, if we're able to make that work... and if not, @crhallberg's idea of inheriting files that are unchanged seems like the next best thing.

@sturkel89
Copy link
Contributor

I have run the full test suite on this PR. Here are the results:

ERRORS!
Tests: 2567, Assertions: 10663, Errors: 37, Failures: 8, Skipped: 64.

Please let me know if you'd like me to paste more detail here in the PR.

@EreMaijala
Copy link
Contributor Author

I've now added a vendor/bin/phing update-bootstrap5-templates task and removed the templates from git. Next step would be to fix pagers.

themes/bootstrap5/js/embedded_record.js Dismissed Show dismissed Hide dismissed
themes/bootstrap5/js/common.js Dismissed Show dismissed Hide dismissed
@demiankatz
Copy link
Member

@EreMaijala, I see that GitHub is complaining about some things in the Javascript. Are these new issues, or is it re-reporting things we had already investigated in bootstrap3?

@crhallberg
Copy link
Contributor

@demiankatz it looks like Javascript that is in bootstrap3

@sturkel89
Copy link
Contributor

I did some additional testing of this branch today and found a couple of new (?) issues with the bootstrap5 and sandal5 themes that don't appear in the legacy themes.

Extra X in search box while typing

I discovered this while testing the Virtual Keyboard but then realized it's an issue even without that feature enabled. In the two new themes, an extra X appears while entering text and seeing search suggestions. There's only one X visible under the same circumstances in the three legacy themes.

GOOD - LEGACY THEMES
Sandal
image

Bootstrap3
image

NOT SO GOOD - NEW THEMES

Sandal5
image

image

Bootstrap5
image

Hebrew example shows different locations of the two Xes
image

@sturkel89
Copy link
Contributor

Facets filtering layout

The layout of the facets lightbox looks different in the new themes vs. the legacy themes. I am not sure that this is a big problem, but it looks strange in the new themes and I think something may be wrong with the styling -- especially since the lightbox behaves better after you resize the browser.

NB: facet sorting, filtering, and clearing the search box all behave normally in every theme.

Examples come from Sandal vs. Sandal5, but the same is seen in Bootstrap3 vs. Bootstrap5.

GOOD BEHAVIOR - LEGACY THEMES
In sandal, the Sort and Filter boxes are aligned in a single row, and the contents of the long table are scrollable within a lightbox that is fully visible on the screen.

When you resize the browser window in the legacy themes, the behavior of the lightbox stays the same – it just gets taller or shorter, as appropriate.

image

QUESTIONABLE BEHAVIOR - NEW THEMES
In sandal5, the Sort options are on one line and the Filter box is on a second line. The contents of the table are in VERY long lightbox that doesn't fit on the page at first.

image

When you resize the browser window, either making it longer or shorter, the scroll bar appears on the right and the CLOSE button pops up from the bottom.

image

This post-resizing appearance/behavior is more in line with what I'd expect. I would prefer that the Sort and Filter options appear on one line at the top of the lightbox in fullscreen.

@sturkel89
Copy link
Contributor

sturkel89 commented May 20, 2024

Responsive table in search history - not fully implemented in new themes

In reference to PR 3204, each item in the search history should be displayed with information in different shades in alternating rows in mobile view. There should be space between the each "recent search", and each item should have a bit of shading to give it a slightly 3D/raised appearance

Search history looks fine in the legacy themes, but in the new themes some of this styling is missing. These screenshots show Bootstrap*, but the same is found in the two versions of Sandal.

Bootstrap3

image

Bootstrap5

image

@EreMaijala
Copy link
Contributor Author

@sturkel89 Thanks for reporting the issues. They should be fixed now.

@EreMaijala EreMaijala requested a review from demiankatz May 21, 2024 14:53
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

I think I may have spotted one minor oversight. Other than that, we may be ready, though I'll give @crhallberg a little longer to comment as well.

@EreMaijala EreMaijala requested a review from demiankatz May 22, 2024 06:11
build.xml Outdated Show resolved Hide resolved
Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

There's no new feedback from me, but I see @crhallberg has caught a few more small things, so I'm going to put this into "request changes" mode until you have a chance to address his feedback.

Copy link
Member

@demiankatz demiankatz left a comment

Choose a reason for hiding this comment

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

Looks like everything we expect to address here has been addressed, and there are a couple of follow-up actions we can take after merging this. Thanks, @crhallberg and @EreMaijala!

@demiankatz demiankatz merged commit 12efd6a into vufind-org:dev May 22, 2024
7 checks passed
@demiankatz demiankatz deleted the dev-bootstrap5 branch May 22, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility architecture pull requests that involve significant refactoring / architectural changes new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants