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

feat(gallery): Improved gallery modal captions - OEL-2129 #509

Open
wants to merge 10 commits into
base: development
Choose a base branch
from

Conversation

tibi2303
Copy link
Collaborator

No description provided.

@github-actions
Copy link

github-actions bot commented Jan 31, 2023

🚀 Deployed on https://preview-509--oelibrary.netlify.app

@github-actions github-actions bot temporarily deployed to pull request January 31, 2023 15:31 Inactive
[skip chromatic]
@github-actions github-actions bot temporarily deployed to pull request February 2, 2023 20:02 Inactive
@@ -14,7 +14,8 @@
image: (string) <img> tag
caption (string)
caption_classes (string)
caption_title (string)
caption_title (string)
caption_visible_mobile (boolean)

Choose a reason for hiding this comment

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

Value is never initialised?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changed the approach, now it's initialised inside the gallery and passed to the carousel.

@@ -17,6 +17,7 @@ module.exports = {
media: `<img alt="First slide"
src="https://picsum.photos/id/1005/800/400"
/>`,
caption_visible_mobile: true,

Choose a reason for hiding this comment

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

Why should I set this when passing the information about the data?

Choose a reason for hiding this comment

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

This should be rather a parameter of the carousel, possibly marked as internal, which hides the caption on mobile. show_caption_mobile or something tied to gallery. Or better, add a block to wrap the needed code and override it when extending the carousel.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't think i need an extra block, i changed the approach and the extra param comes from gallery directly and not from each single item.

@@ -170,8 +170,6 @@ Parameters:
{% block carousel %}
{% for _item in _items %}
{% set _carousel_items = _carousel_items|merge([_item|merge({
caption_title: ('<iframe' in _item.media or '<video' in _item.media) ? '' : _item.caption_title,
caption: ('<iframe' in _item.media or '<video' in _item.media) ? '' : _item.caption,
image: ('<iframe' in _item.media or '<img' in _item.media) ? _item.media|replace({'src=': 'data-src='}) : _item.media,

Choose a reason for hiding this comment

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

Side note: this breaks if there's another attribute like <img data-thumbnail-src="...">.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added an extra space to be sure src is as alone without any extra data-. Should be ok i think

@github-actions github-actions bot temporarily deployed to pull request May 11, 2023 06:59 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 11, 2023 07:38 Inactive
Tiberiu Dumitru added 2 commits May 11, 2023 15:34
[skip chromatic]
@github-actions github-actions bot temporarily deployed to pull request May 11, 2023 12:45 Inactive
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.

2 participants