-
-
Notifications
You must be signed in to change notification settings - Fork 14
Update featured story carousel #188
Update featured story carousel #188
Conversation
Signed-off-by: Eric Poirier <eric.poirier@eclipse-foundation.org>
✔️ Deploy Preview for eclipsefdn-ecdtools ready! 🔨 Explore the source changes: 4b9d735 🔍 Inspect the deploy log: https://app.netlify.com/sites/eclipsefdn-ecdtools/deploys/61969c97a1ed810008062ac4 😎 Browse the preview: https://deploy-preview-188--eclipsefdn-ecdtools.netlify.app/ |
Signed-off-by: Eric Poirier <eric.poirier@eclipse-foundation.org>
Signed-off-by: Eric Poirier <eric.poirier@eclipse-foundation.org>
Signed-off-by: Eric Poirier <eric.poirier@eclipse-foundation.org>
Signed-off-by: Eric Poirier <eric.poirier@eclipse-foundation.org>
We have the +1 from Shanda |
{{< home/powered-by >}} | ||
{{< home/highlights >}} | ||
|
||
{{< home/highlights |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a component that we would want to re-use on other sites?
If so, we should contribute to the base theme as I did with the search page: https://gitlab.eclipse.org/eclipsefdn/it/webdev/hugo-solstice-theme/-/merge_requests/246
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I'll try to make that as generic as possible.
This is a featured story block that is pulling multiple items. I could create a shortcode called "featured-story-multiple" on hugo theme. I don't think we should modify the existing featured-story shortcode since the html is not the same in the 2 cases.
What do you think?
publishTarget="ecd_tools" | ||
templateId="featured-story-custom" | ||
count="5" | ||
templatePath="/js/templates/featured-story-custom.mustache" >}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to stay consistent with your naming conversation. You are calling this home/highlights but the template is featured-story-custom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we need to be consistent. I'll rename the template based on what the shortcode will be named.
|
||
$("body").on("shown.ef.featured_story", function(e) { | ||
var owl = $('.solstice-slider-home'); | ||
owl.trigger('destroy.owl.carousel'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you trigger a destroy and refresh event. I feel like we are doing something wrong here. I don't really understand.
Adding comments in your code would help us understand if this is really needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok i'll add some comments in there.
The destroy and refresh here helps because owl carousel needs to be updated after the items have been added to the container div with ajax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about you try to load the data before initializing owl?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make more sense, I'll try that.
As discussed, you can merge this so that we can solve the problem but I will expect the code to be improved when we have a bit more free time. |
I created the following issue to export this functionality from ECDtools to Hugo theme. |
#165
Signed-off-by: Eric Poirier eric.poirier@eclipse-foundation.org