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

Replace Slick Carousel with accessible-slick #2702

Closed

Conversation

stephanieleary
Copy link
Contributor

accessible-slick by Accessible360
(https://github.com/Accessible360/accessible-slick) is a drop-in replacement for the original Slick Carousel.

accessible-slick by Accessible360
(https://github.com/Accessible360/accessible-slick) is a drop-in
replacement for the original Slick Carousel.
@demiankatz
Copy link
Member

Thanks, @stephanieleary! Could you take a look at this, @crhallberg?

@demiankatz
Copy link
Member

@stephanieleary -- I notice that the old slick.min.js includes a header with version number and other details, but the new version does not. Maybe this is just a shortcoming of Accessible Slick... but it would be helpful if we had a way of keeping track of what version is installed, and also of attributing the authorship/license of the vendor code. Am I missing something in these existing files? Is there something we can/should add to make this more clear?

I wonder if it might also be worth investigating adding this as a dependency in package.json, and then copying the appropriate files into place with a Phing task, if that's a feasible approach to installation. We've done that for some (but not yet all) of our other JS dependencies, and if it's possible, it might be a nice step in the right direction that we could take care of at the same time that we're improving accessibility. I can help with details if necessary. :-)

Copy link
Contributor

@crhallberg crhallberg left a comment

Choose a reason for hiding this comment

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

  1. I agree that a comment somewhere that points to the repository and current version of accessible-slick would be very helpful. We have header comments for the other items, so that's one good way.
  2. We'll need a LESS version for development. Even if it's a version run through a SASS to LESS translator. If issues come up with variables or file paths, we have mechanisms to deal with this.

@demiankatz
Copy link
Member

@crhallberg, I don't think LESS files are strictly necessary in this case, because we're including the compiled CSS directly rather than building it ourselves -- the SCSS files are there as part of the vendor package but, as far as I can tell, are not actually being used for anything.

@stephanieleary
Copy link
Contributor Author

That's correct; I included the LESS files from the package, but they're not required.

This fork does not include the version header that the original Slick had, and I agree that we need something to indicate where this came from. I suppose stashing the readme somewhere is an option, but I like the idea of using package.json for this.

@demiankatz
Copy link
Member

@stephanieleary, if you'd like to try the package.json approach, you can use our cookie consent dependency as a model.

We install it as a dev dependency here: https://github.com/vufind-org/vufind/blob/dev/package.json#L24 (I believe we chose dev because we only need to download this when we're doing a build, since the file will also be committed to Git, and "normal" users won't have to take any special action).

We copy the files into the appropriate place as part of the copynodemodules Phing action: https://digital.library.villanova.edu/Collection/vudl:313871

Then it should just be a matter of running npm install followed by vendor/bin/phing copynodemodules to get everything up to date.

Let me know if you need any help!

@crhallberg
Copy link
Contributor

We brought this up on a call and one our contributors noted that they actually moved from accessible-slick to the even more accessible Splide recently. It supports some features that we struggled with in Slick, and if we think it's better to go to this library instead, I'm happy to lead the effort.

@demiankatz demiankatz added this to the 9.1 milestone May 2, 2023
@demiankatz
Copy link
Member

I've merged the latest dev branch into this PR in an effort to get us a green checkmark, but unless @stephanieleary objects, it looks to me like it might be worth closing this and putting some effort into Splide instead.

@stephanieleary
Copy link
Contributor Author

That sounds good to me! I reached for this package because it was a drop-in replacement for Slick, and I was pressed for time.

@EreMaijala
Copy link
Contributor

Unfortunately I don't have suitable code to share since we don't use channels at the moment but implemented Splide with an RSS carousel.

@demiankatz
Copy link
Member

Thanks for checking, @EreMaijala. Looks like we're going to have to take @crhallberg up on his offer of establishing a new Splide PR to replace this one. I'll leave this open as a reminder until that PR can be created, and then we can close this one.

@demiankatz
Copy link
Member

Thanks for opening #2934, @crhallberg! I'm closing this now.

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.

5 participants