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

Switch to Splide Carousel Library #2934

Open
wants to merge 56 commits into
base: dev
Choose a base branch
from

Conversation

crhallberg
Copy link
Contributor

@crhallberg crhallberg commented Jun 7, 2023

Continuation of #2702.

  • npm install
  • Channels implementation
  • use for SimilarItems carousel
  • remove jQuery from carousels setup while we're refactoring done in common.js, the rest can be saved for de-jQuery project.

@demiankatz
Copy link
Member

Thanks, @crhallberg! I'll test this soon when time permits. In the meantime, is it possible to install splide via npm as we've done with some of our other dependencies, or is that not feasible?

@demiankatz demiankatz added this to the 9.1 milestone Jun 7, 2023
@demiankatz
Copy link
Member

@crhallberg, I never looked closely enough at #2702 to realize that this was about the Channels view and not the SimilarItems carousel. Is that using native Bootstrap or something else? Should we consider converting that to Splide as well for consistency?

@crhallberg
Copy link
Contributor Author

Yes! I intend to replace all of the carousels, I just wanted to get the Channels one down first.

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.

Thanks for getting the ball rolling, @crhallberg. This doesn't seem to be working yet for me, though. When I go to the channels page, I get this error:

Uncaught Error: [splide] A track/list element is missing.
    bn assert.ts:12
    p Elements.ts:181
    t Splide.ts:126
    w forOwn.ts:20
    w forOwn.ts:19
    mount Splide.ts:123
    setupChannelSlider channels.js:96
    jQuery 2
    init channels.js:191
    init common.js:113
    commonDocReady common.js:654
    jQuery 13
assert.ts:12:10

And I end up with a partially-rendered screen like this:

image

Any idea what's going on?

Also, see below for an unrelated question.

package.json Outdated Show resolved Hide resolved
@demiankatz demiankatz modified the milestones: 9.1, 10.0 Aug 1, 2023
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 was trying to find our conversation with @EreMaijala where I thought we made a decision about dependencies vs. devDependencies in package.json, so that I could reference it in a different review. I didn't find it, but I did notice a couple of problems here during the search. I haven't done a thorough review of this yet, but I'm leaving the review before I forget about these things.

config/constants.config.php Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@demiankatz demiankatz added architecture pull requests that involve significant refactoring / architectural changes and removed improvement labels Nov 7, 2023
@crhallberg
Copy link
Contributor Author

@demiankatz @sturkel89 I was not able to see the Sandal not working error. I did update the mobile styles however.

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.

@crhallberg,

I took another look at this. I'm still seeing the sandal problem (at least in Firefox, my usual browser for testing): channels load slowly, if at all, in Sandal, even though they seem normal in bootprint3 and bootstrap3. Here's a screen shot of my latest attempt at Sandal -- note the "page is slowing down your browser" warning -- something is causing Javascript to do heavy processing, but only in one theme!

image

I also notice that npm run build:css is throwing errors about missing include files in the bootstrap5 theme -- I suspect that significant work will be needed to reconcile the bootstrap3 and bootstrap5 versions of the conversion. @EreMaijala might have thoughts on how complex this is likely to be.

In any case, all of this adds up to suggest to me that we're not going to get this done in time for release 10.0 -- but we can discuss on today's Community Call!

@EreMaijala
Copy link
Contributor

Oh no, @pasitiis noticed that there haven't been new releases for Splide since 2022, and the last commit is also more than a year ago. Seems like the development may have stalled. Apparently https://swiperjs.com/ might be an alternative, but can't vouch for it or any other.

@demiankatz
Copy link
Member

Argh, that's quite concerning. Once again, it seems like abstraction needs to be the order of the day, since we may need to hot-swap carousels from time to time. @crhallberg, do you think there's work here that might be able to be generalized? Should we, for example, open a separate PR to introduce the vf-carousel classes independently of the Splide upgrade? I think given the size of the change here and the proximity of the release, we probably should delay going forward with what we have here until we can get more information. Do you disagree?

@crhallberg
Copy link
Contributor Author

@demiankatz I don't disagree. The vf-carousel changes were necessary to separate the carousels from Bootstrap. This should definitely be abstracted into a component like menu-button.

@EreMaijala Swiper looks like a good replacement!

@demiankatz
Copy link
Member

I'm pushing all theme-related PRs that were not finished in time for 10.0 to the 11.0 milestone; it will be easier to finish these if we only have to operate on one theme at a time. I think we should establish a dev-11.0 branch fairly early in the 10.1 development process so that this work can move forward as soon as possible.

@demiankatz demiankatz modified the milestones: 10.0, 11.0 Jun 14, 2024
EreMaijala pushed a commit to EreMaijala/vufind that referenced this pull request Aug 27, 2024
@demiankatz demiankatz changed the base branch from dev to dev-11.0 August 27, 2024 10:44
@pasitiis
Copy link
Contributor

pasitiis commented Oct 3, 2024

We have been using Splide in Finna for a few years, but it’s not perfect from an accessibility perspective for our needs. Although Splide follows the ARIA carousel pattern, using it with a screen reader is somewhat awkward for browsing items. It would be better to forgo the carousel pattern altogether, structure it as a list, and create a carousel-like appearance, either with or without pagination.

I could be interested in helping, regardless of which component the work continues with.

@demiankatz
Copy link
Member

I like the idea of a styled list, if doing that correctly isn't terribly burdensome. Replacing a third-party library with standard HTML worked well for hierarchy trees, so maybe carousels could be handled similarly. I defer to @crhallberg on this decision, though, as he probably has a better sense than I do of exactly how complex this might end up becoming. :-)

@crhallberg
Copy link
Contributor Author

I do agree that a styled list would be much more accessible for screen readers and would work well on mobile where horizontal scrolling is increasingly common. However, we would need a solution for desktop. Splide not being the solution is fine, this PR has some good value of making it easier to replace the library with another. cc @demiankatz @pasitiis

@crhallberg
Copy link
Contributor Author

I am hesitant to return to rolling our own carousel, but we can piggyback some work at least. W3m has an example accessible carousel. We could start with that code and add some modern touches such as CSS scroll snapping.

@demiankatz demiankatz changed the base branch from dev-11.0 to dev November 1, 2024 17:52
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants