Add carousel option to the posts

[Letme]

This pull request adds an option to add a carousel after your post content to any page. Carousel is a gallery-like slideshow, here I have styled it as the main photo and a bunch of smaller photos below and I have used the tiny-slider library, so that I did not have to write the jquery for swapping images.

Settings for tiny-slider could be modified from the configuration in the future, but at the moment I left it hardcoded the way I have it.

Images are free images pulled from pexels.com, but in case you have any of your own (I have seen you are a photographer) feel free to edit the Pull Request with them.

The path to the images (../imagename.png) is required to be like that, because each markdown file is treated as a directory when Hugo creates it, so if you want to reference images from your markdown directly you will need to add that ../ in front of the image name. Maybe it's a bug, but for me, it is the way it is.

I could not find the way to edit the documentation as well (the docs linked here in the README.md), so please tell me where to add the description of this new Front Matter variables.

It's my first contribution to anything related to Hugo or Go templating for this matter, so I do not mind fixing it until it is OK. I might also have forgotten some of the changes when I copied them from my website.

[wangchucheng]

Hi,

It is an amazing feature based on the description! But when I tried to use it, I found you add carousel in layouts/partials/components/doc-layout. This is the layout for a doc page. And the template for single post page is in layouts/_default/single.html.

If you want to use this feature in both doc and post pages, you need to add it in both of them. Currently this is a bit redundant but we will improve it in the future.

I tried to paste these codes to layouts/_default/single.html:

                    {{ if isset $.Params "carousel" }}
                    {{ if $.Params.carousel.enable }}
                    {{ if gt (len $.Params.carousel.images) 0 }}
                    {{ partial "components/carousel" . }}
                    {{ end }}
                    {{ end }}
                    {{ end }}

But when I run it, an error occurs:

ERROR 2021/01/24 10:23:22 Failed to render pages: render of "page" failed: "<my-path>\themes\eureka\layouts\_default\single.html:23:43": execute of template failed: template: _default/single.html:23:23: executing "main" at <partial "components/carousel" .>: error calling partial: "<my-path>\themes\eureka\layouts\partials\components\carousel.html:23:43": execute of template failed: template: partials/components/carousel.html:23:43: executing "partials/components/carousel.html" at <$angleleft.Permalink>: nil pointer evaluating resource.Resource.Permalink    

Did I do something wrong? Can you fix this problem first and then we can start to discuss the codes in detail. Thank you for your contribution to Eureka!

[Letme]

Ok, I did not know about the posts single.html file. I have added that in 4333e8a.

I see that the arrow images are missing, so I added them in 3da8b4f .

I tried to build example website by creating themes/eureka and symlinking it to the project root (see below - maybe you can add it to readme, as this seems like required step to build exampleSite).

cd exampleSite
mkdir themes
ln -s ../.. themes/eureka

Then I checked the output and images also end up in public/posts (I needed to clean the public I guess). So now I would need something to serve the exampleSite to see proper paths and all - how do you usually do that, do you spin up some nginx?

[wangchucheng]

Thank you for your contribution. I have made some suggestions, but these are only my personal opinions. If you think there is any better way please feel free to suggest.

I tried to build example website by creating themes/eureka and symlinking it to the project root

If you want to run the exampleSite, you can just do the following in your project:

cd themes/eureka/exampleSite
hugo server --themesDir ../..

Then I checked the output and images also end up in public/posts (I needed to clean the public I guess).

And for images in posts folder. You can create a folder named carousel in this case and rename carousel.md to index.md and move it to carousel folder along with those images. And images will be in localhost:1313/posts/carousel/carousel1.jpg for example.

In addition, I have extracted the eureka documentation into a new repo hugo-eureka-docs for the documentation, where you can add content for this feature.

[wangchucheng]

Do you think it would be a good idea if we put tinyslider-controls before tinyslider-carousel?

I found that if the aspect ratio of the picture changes, the tinyslider-controls will move up and down. Placing the tinyslider-controls above the picture can avoid this problem.

[Letme]

It's a style preference of course, but that moving up and down is basically determined by autoHeight in the tns object in carousel.html. Like I said, we can make those options configurable and maybe the position of carousel in subsequent PR?

<script>  const slider = tns({
		container: '#tinyslider-carousel',
		items: 1,
		autoplay: false,
		center: true,
		nav: true,
		navContainer: "#customize-thumbnails",
		controlsContainer: "#customize-controls",
		navAsThumbnails: true,
		autoHeight: true,
	  });
	</script>

[wangchucheng]

What do you think about the relationship between featuredImage and carousel? I am thinking whether there is a use case where these two attributes are used at the same time? If not, we can merge it into one attribute.

Btw, the enable is not necessary because you can use {{ if .Params.carousel }} or {{ with .Params.carousel }} to judge.

And I think this can be refactorred as carousel = ["image_url_1", "image_url_2"]. The order is weight. and for others who are willing to add a description, carousel = [{url= "carousel1.jpg", description = "Caption of the first image." }, ...] can be used. And use functions like reflect.IsMap in template to determine whether the item in the list is a string or a map.

[Letme]

Agreed. I assume something like this was in your mind: fabf93a

[wangchucheng]

As comment in carousel.md, a single {{ if .Params.carousel }} or {{ with .Params.carousel }} is great.

[Letme]

Done in fabf93a

[wangchucheng]

Please do not hard code assets. You can define it in data/assets.toml and call it with something like {{ printf $assets.highlightjs.js.url $assets.highlightjs.version }}. You can refer to partials/head.html:78 for more details. And please use jsdelivr if it is possible. This theme has many users in China and jsdelivr performs much better than cloudflare in China.

And please use async or defer for script if it is possible.

[Letme]

Could not add defer or async as object is called right at site creation. When I tried the console reported tns object not defined...

Changed to usage of assets.toml c1e6f71

[wangchucheng]

Same as single.html

[Letme]

Done in fabf93a

[wangchucheng]

The if conditions can be simplified as described above. And please define css in data/assets.toml and call it here.

[Letme]

Conditions simplified in fabf93a

Defined in assets.toml in c1e6f71

[wangchucheng]

I can not comment on an image so I comment here.

Do you think it is possible to use an icon instead of an image here? It will not only increase the repo size (Hugo has limitations on theme repo size) but also increase the time users wait for the page to load.

If using an icon is possible, font awesome may be a good choice because we already use icons from font awesome so users will not need to wait for another package to load. For example, angle-right may be a good option. You just need change it to <i class="fas fa-angle-right"></i> to use it.

[Letme]

Great, did not think that fonts have arrows defined. So changed in c89b1fd

[wangchucheng]

I can not comment on an image so I comment here.

At present, the newly added images are too large, due to Hugo's restrictions on the size of theme repo. In order to better reuse the existing pictures in the project, you can use the two pictures in the theme assets folder. If you use utils/get-image to get image url, you can get these images very easy by setting front matter like images/hero-left.jpg.

If you think that two pictures are not enough to show this feature, I can also provide one or two pictures for use.

[Letme]

I think we need at least 4 pictures, so that we can show the scrolling and looping. Otherwise we just see swap of two pictures. Feel free to add them to my repo/branch if you have any better. I can severely downgrade quality of the images anyway, as they are only used for demonstration.

[Letme]

In addition, I have extracted the eureka documentation into a new repo hugo-eureka-docs for the documentation, where you can add content for this feature.

This repo needs LICENSE file otherwise GitHub is not allowing me to open a Pull Request against (I assume).

[wangchucheng]

I have made some suggestions below. And for the config issue, I agree with you that we can finish this feature first then we can consider to make it configurable.

Also, I have added the same LICENSE as Hugo Eureka in Hugo Eureka Docs repo.

Btw, do you know if tiny-slider supports videos? I see a similar package called Splide.js can support video with an extension.

[wangchucheng]

That is exactly what I want. In order to unify the format, I think the image can be renamed to url

Besides, how about combining featuredImage and carousel into one attribute? When the new featuredImage is a string or an array which only contains one item, it will be the old featuredImage. And if this attribute has more than one item, carousel will be used. What do you think of this idea?

[Letme]

I am not a fan of too much automatization as I see a use case where featured image is not part of the carousel set. For example in featured image you make something totally related to title, to attract a reader, but then in carousel you just have images of details.

Based on below proposal of adding a video, I would leave image as that would enable us to use video and swap the text inside div from <img> to <video>

[Letme]

Btw, do you know if tiny-slider supports videos? I see a similar package called Splide.js can support video with an extension.

It is basically just scrolling divs, so if we decide on the videos, then we could swap the <img> tag inside with the embedded video tag (either html5 or youtube or iframe). In that case dictionary seems like way more flexible way as we can define if item in carousel is video or image

[Letme]

I have added this get-image function calls (3a07d8f), yet the URL it produces is wrong as images are in public/posts yet this function resolves it to root .

[wangchucheng]

As we discussed before, there is a mistake in organizing images in content folder. You can create a folder named carousel in this case and rename carousel.md to index.md and move it to carousel folder along with those images. And images will be in localhost:1313/posts/carousel/carousel1.jpg for example. The front matter should be carousel1.jpg for example. Please note that the images you uploaded is jpg, but the filename you entered is png.

This is called Leaf Bundles in Hugo.

And if you have done that, get-image should give you a correct result as I tested it in my machine.

[Letme]

Done this in 39b7b9c .

[wangchucheng]

For video tests

I tested carousel with videos and it seems that we need a thumbnail of each video. Otherwise, the part of tinyslider-controls will not work properly. Because each item is a video so we can not click it to switch unless we click the border.

So I think we can just focus on image for this pr. Videos may need more configurations.

For the featuredImage and carousel issue

And for the featuredImage and carousel issue, the use case you mentioned does exist. But after discussing with the co-author of this theme (although I commited most codes, we design this theme together), we decide that featuredImage and carousel should be combined into one attribute for the following reasons:

1. The carousel is currently located at the bottom of the page, but for pictures, most websites will place it at the top of the page. We also think that placing it on the top is a more intuitive way. But if the carousel is placed on the top, it conflicts with the featuredImage.
2. Eureka hopes to provide users with a concise configuration. We are more restrained in introducing new configurations. In our opinion, allowing users to add multiple featuredImages may be a broader requirement. And providing a carousel at the top can also solve the needs of most users for a carousel.

[Letme]

OK, care to explain how do you check if it is a string (something featuredImage is currently) or array (or map) in Hugo?

I did some googling and one idea is len, but that doesn't work as it simply returns length of string. I tried reflect.isString but also is not supported, so some help for this would be great. I have moved a call to carousel partial inside the utils/get-featured and I will make it a standard template instead of return. Do you see any problems with that approach for other functionality?

I can then add implementation of the position of featuredImage into the config/params.toml to put it below content or above content

[wangchucheng]

care to explain how do you check if it is a string (something featuredImage is currently) or array (or map) in Hugo?

I think reflect.IsSlice should work. Because an array is a slice in Hugo which is a little bit weird at least for me.

Do you see any problems with that approach for other functionality?

What I can think of for now is that you may also need to modify summary pages, such as the list page (/posts/, etc.). Those pages will display the featured image and may not be able to handle carousel now.

I can then add implementation of the position of featuredImage into the config/params.toml to put it below content or above content

How do you plan to configure it? You can put forward an idea first and then we can have more discussions about it.

[Letme]

I tried with reflect.isSlice yet it returns true even when its not array. Could we rather drop the array thingy and use reflect.isMap to use the dictionary approach?

I did some of them and also intputs to get-image function will have to be adjusted everywhere where featuredImage is passed to something like

{{ if isset .Params "featuredImage}}
{{- $img := partial "utils/get-image" (dict "context" . "url" (first 1 .Params.featuredImage) "keyword" "*featured*") }}
some HTML stuff with $img
{{- end }}

Here is a proposal of a configurable position for carousel: c18eed8 . You can rename it to featuredImagePosition when we rename it to featuredImage

[wangchucheng]

I tried with reflect.isSlice yet it returns true even when its not array.

You can use it as following and it may work:

{{ if reflect.IsSlice .Params.carousel }}
    ...
    {{ range .Params.carousel }}
    ...
        {{ if reflect.IsMap . }}
        ...
        {{ end }}
    ...
    {{ end }}
...
{{ end }}

everywhere where featuredImage is passed to something like

Is it possible to use carousel in summary? Putting arrows above the image or using small dots below or something similar. But if it is hard to implement, we can just use the first image of the array.

Here is a proposal of a configurable position for carousel: c18eed8

Thanks for the proposal! We will discuss whether to add it or where to add it and share our opinion with you ASAP.

[Letme]

I tried exactly that, except for params.featuredImage and it broke on several places. But it is true, that I wanted to in fact extract first image only, not produce a carousel in summary. Also schema partials have image, and there can be only one (first). I can push what I have for you to look, but its not compiling...

The carousel style which is configurable has a wide range. From arrows, to no-arrows, to small dots on top (http://ganlanyuan.github.io/tiny-slider/demo/#center_wrapper). I would imagine that in summary these small dots on top would indeed look cool. Tell what you decide and I can enhance.

[wangchucheng]

I tried exactly that, except for params.featuredImage

Maybe a {{ end }} position issue? I tested it in my machine and it works. If this is still unresolved, you can push it and I will see if there is anything I can help you.

I would imagine that in summary these small dots

If this is not hard to implement I think we can try it. The carousel is at the top of summary and dots is below images in carousel.

Also schema partials have image, and there can be only one (first).

Yes, we can use the first image for the image attribute as schema only allow one image as far as I know.

[Letme]

Ok, so non working commit which needs your suggestion is in: 3bc5853 . A bit lost there...

[wangchucheng]

Ok, so non working commit which needs your suggestion is in: 3bc5853 .

There are some small mistakes here:

1. The correct function name is reflect.IsSlice not .reflect.isSlice.
2. Line4 is $isCarousel = reflect.IsSlice .Params.featuredImage not $isCarousel := .reflect.isSlice .Params.featuredImage. = is to assign value and := is to define a new variable. Using := inside an if causes the variable can not be accessed outside. So in this case, a = should be used. See Variables for more details.
3. Line11 is partial "components/carousel" . not partial "components/carousel" .Params.featuredImage. Based on what you wrote in components/carousel, you need to pass all the context to this partial not just .Params.featuredImage. Because if you pass .Params.featuredImage only, the context(.) will be rebinded to .Params.featuredImage which may cause errors.

After fixing this, you can run without any errors. And I also wrote another sample for you to reference (I haven't tested every case of it, but currently it works):

{{ if .Params.featuredVideo }}
  {{ partial "utils/get-video" (dict "context" . "url" .Params.featuredVideo "keyword" "*featured*") }}
{{ else if .Params.featuredImage }}
  {{ $image := "" }}
  {{ if and (reflect.IsSlice .Params.featuredImage) (gt (len .Params.featuredImage) 1) }}
    {{ partial "components/carousel" . }}
  {{ else if reflect.IsSlice .Params.featuredImage }}
    {{ $image = partial "utils/get-image" (dict "context" . "url" (first 1 .Params.featuredImage) "keyword" "*featured*") }}
  {{ else }}
    {{ $image = partial "utils/get-image" (dict "context" . "url" .Params.featuredImage "keyword" "*featured*") }}
  {{ end }}
  {{ if $image }}
    <img src="{{ $image }}" class="w-full" alt="Featured Image">
  {{ end }}
{{ end }}

[wangchucheng]

And for the proposal of configuring the location of featured media, although we think that placing this section above the content is what most users need, we also hope to listen to the community’s suggestions as comprehensively as possible. So if you'd like to implement this, you can do this in another pr if you like. And if many users need this pr, we may consider merging it.

[Letme]

OK, so I have implemented the featured image and the changes in 58424c7 . What really got me off track is that Page params can only be called in lowercase, so .Param.featuredImage should be .Param.featuredimage (blue bar: https://gohugo.io/variables/page/#page-level-params). After that it all got working.

Now I left the style as it was (with images below), but I would start on another PR for configuration of this. In current state I cannot use it anyway, but if I add more stuff this is going to become too big in my opinion. Also you changed toml to yaml, which makes a mess if I would have a lot more changes.

If you agree after this PR I promise to make: Configuration PR and position of featured image PR (I have commit somewhere already).

[wangchucheng]

Sorry for the late reply, I just tested your commits on the add_carousel branch. But encountered an error:

Error: Error building site: failed to render pages: render of "page" failed: execute of template failed: template: _default/single.html:3:4: executing "_default/single.html" at <partial "head" .>: error calling partial: "my-path\themes\eureka\layouts\partials\head.html:60:55": execute of template failed: template: partials/head.html:60:55: executing "partials/head.html" at <len .Params.featuredimage>: error calling len: reflect: call of reflect.Value.Type on zero Value

I cannot comment at the moment because I have not seen the current results. But no matter what the result is, thank you very much for your continued contribution.

[Letme]

Ok, it took me quite a while, but I managed to find the problem in 4f0ffba .

[wangchucheng]

Hi,

Sorry for not being able to reply in time in the past period of time. It was Chinese New Year before and there are many personal affairs I need to deal with after the holidays, I just had time to test those new commits.

Functional Issues

One problem is that there are some offsets of images in single page like:

Besides that, this new feature works well in single page. But the featured image can not be displayed on list page for now (such as /posts).

and

These are the two problems I have found so far. I think we can also start discussing style issues.

Style Issues

I think the border of this thumbnail is unsightly, for example, the border and the thumbnail are not aligned. And the border style does not match the theme for now.

In addition, I noticed that the current style is written directly using css. However, Eureka uses tailwindcss for encapsulation, so please use tailwindcss to write styles if possible.

[Letme]

No problem. Happy New Year 😉

Functional issues

Offset for the style

done: aaf64ec as removal of the w-full

List not displaying the featured image

Fixed the posts featured image d648de6 - wondering if also carousels should put the first image as featured?

Style Issues

Border around image selector

Strange, I do not have that in Chrome. Which browser are you using? Can you try again - maybe with the offset for the style something else changed as well?

tailwindcss for encapsulation,

Honestly, the only CSS in here is needed for the tiny-slider library. I doubt I can make it much more different...

[wangchucheng]

wondering if also carousels should put the first image as featured?

Great!

Strange, I do not have that in Chrome. Which browser are you using?

I also use Chrome and I found that this is a problem that only exists at some resolutions. I tried multiple resolutions:

Only some resolutions have white borders.

Honestly, the only CSS in here is needed for the tiny-slider library. I doubt I can make it much more different

Yes, what I want to say is that these styles can be written in a tailwind way. If you don't know much about tailwind, I can modify it before merge.

[Letme]

Added as featured image in 036bed5

No idea how to spot what you see. I was dragging the "responsive" up to 1404 width and then when I drag back together it is always nicely fitting the image - no borders at all. Since you can reproduce, could you also debug what css maybe the parent div needs or just tiny-slider needs?

I have no idea about tailwind way and it is honestly something I would have to learn. If you can do a quick fix of that I am totally happy.

[Letme]

This seems to be stuck a bit. And some conflicts. Can I make it progress somehow?

[wangchucheng]

Sorry for not responding yet because I haven't solved the border style problem for now. It will be very great If you want to solve those conflicts. And since I am writing a paper recently, the fix of the border style problem may also be delayed. So if you also solve this, please let me know and I will try to review it ASAP.

…

________________________________ 发件人: Crt Mori ***@***.***> 发送时间: Thursday, April 29, 2021 6:37:14 AM 收件人: wangchucheng/hugo-eureka ***@***.***> 抄送: C. Wang ***@***.***>; Comment ***@***.***> 主题: Re: [wangchucheng/hugo-eureka] Add carousel option to the posts (#32) This seems to be stuck a bit. And some conflicts. Can I make it progress somehow? ― You are receiving this because you commented. Reply to this email directly, view it on GitHub<#32 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AJEXGNHYYLCHADNKOBNXKDLTLCEZVANCNFSM4WP65VFA>.

[Letme]

Ping on this a bit. I am getting back to this project as a regular update cycle for my website so would love to get this in to ease that process a bit in the future.