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

initial setup on thumbnail carousel with working example #198

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

ankitatechie
Copy link

Initial setup of thumbnail carousel with a working example.

It includes the https://splidejs.com/ library as a npm package. I have already setup one working example along with a new page URL i.e. /thumbnail-carousel.php

initial setup on thumbnail carousel with working example
@@ -168,6 +168,8 @@ const nodeFiles = [
'node_modules/indent.js/lib/indent.min.js',
<b>'node_modules/glider-js/glider.js',
'node_modules/glider-js/glider.css',</b>
<b>'node_modules/@splidejs/splide/dist/js/splide.min.js',
'node_modules/@splidejs/splide/dist/css/splide.min.css',</b>
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should not be bolded. The paragraph above states:

"For example, when I added the glider-js library to Enable so I can use it in the Carousel demos, I added the files I needed for the front-end to nodeFiles with these two lines:"

Technically, what you have worked on is not a general carousel. It's an image gallery. We should change this PR to not use the word carousel, but image gallery. I'll make appropriate comments to reflect what we should change.

@@ -197,6 +197,9 @@ js/modules/combobox.js:
js/modules/enable-carousel.js:
16: import '../node_modules/glider-js/glider.js';

js/modules/enable-thumbnail-carousel.js:
16: import '../node_modules/@splidejs/splide/dist/js/splide.min.js';

Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot this file exists. Please delete it. It is not needed anymore (all tickets are now in github Issues).

@@ -0,0 +1,73 @@
<section class="enable-thumbnail-carousel" aria-labelledby="thumbnail-carousel-heading">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's call this page thumbnail-gallery.php

<ul class="splide__list">
<li class="splide__slide">
<img src="images/carousel-example/00-turkish-spider-man.jpg"
alt="Bootleg versions of Spider-Man, Captain America and El Santo fighting." />
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this example, I'd like to use product shots for something. I don't want to use the same images from another example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Change this image with the new images mentioned above.

thumbnails.mount()
}

export default EnableThumbnailCarousel;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure where you'd change this, but I'd like to add the following changes:

  1. When the user tabs into the thumbnail, it reads "Go to slide ${n}". Can you change this to "Display slide ${n}"?
  2. When you click on a thumbnail, the aria-live region just reads the alt. Can you make it read "Now viewing: ${alt}".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants