Skip to content

Conversation

@Meng-Heng
Copy link
Collaborator

@Meng-Heng Meng-Heng commented Nov 25, 2024

follow: #1712

video sample:

video.mp4
  • Should we use the <iframe> tag or use js onclick to set the <iframe>?

-- updated on 09-Dec-2024
Fixes: #1712

@Meng-Heng Meng-Heng added the feat label Nov 25, 2024
@Meng-Heng Meng-Heng added this to the A18S16 milestone Nov 25, 2024
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

I like the idea of not embedding the video until a img preview is clicked; we'll need to tweak the code slightly to get it to work across multiple thumbnails

(This may not be relevant with the requested change, but as a side note, the getElementById() call result was not checked -- if a page did not have a matching element, it would return null, causing an error on t.addEventListener line.)

Comment on lines 2 to 3
let t = document.getElementById("keymanThumbnails");
t.addEventListener("click", function() {
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to use querySelectorAll instead of getElementById, and match on any element that has the appropriate data-video attribute, so we can have multiple thumbnails in the same document. That means then we need to add an event listener for each element found, in a loop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I'll look into this.

@Meng-Heng Meng-Heng requested a review from mcdurdin December 6, 2024 06:47
@darcywong00 darcywong00 modified the milestones: A18S16, A18S17 Dec 7, 2024
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

This is looking good. A few minor style changes requested!


* [Character Map + Touch Layout Editor integration improvements](https://www.youtube.com/watch?v=qoLpuah72kw) (2:15)

<img id="keymanThumbnails" data-video="https://www.youtube.com/embed/qoLpuah72kw" src="https://img.youtube.com/vi/qoLpuah72kw/maxresdefault.jpg" width="600px">
Copy link
Member

Choose a reason for hiding this comment

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

  • id should not be repeated across multiple elements -- perhaps use class instead.
  • Instead of keymanThumbnails can we use video-thumbnail?
Suggested change
<img id="keymanThumbnails" data-video="https://www.youtube.com/embed/qoLpuah72kw" src="https://img.youtube.com/vi/qoLpuah72kw/maxresdefault.jpg" width="600px">
<img class="video-thumbnail" data-video="https://www.youtube.com/embed/qoLpuah72kw" src="https://img.youtube.com/vi/qoLpuah72kw/maxresdefault.jpg" width="600px">

Comment on lines 1 to 13
const thumbnailsList = document.querySelectorAll("#keymanThumbnails");

window.addEventListener('DOMContentLoaded', () => {
thumbnailsList.forEach(each => {
each.addEventListener('click', () => {

if(each.hasAttribute('data-video')) {
let videoId = each.getAttribute('data-video')
each.outerHTML = "<iframe width='600' height='315' src='" + videoId + "' allowfullscreen></iframe>";
}
})
})
}) No newline at end of file
Copy link
Member

Choose a reason for hiding this comment

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

Use an IIFE to prevent pollution of global namespace:

Suggested change
const thumbnailsList = document.querySelectorAll("#keymanThumbnails");
window.addEventListener('DOMContentLoaded', () => {
thumbnailsList.forEach(each => {
each.addEventListener('click', () => {
if(each.hasAttribute('data-video')) {
let videoId = each.getAttribute('data-video')
each.outerHTML = "<iframe width='600' height='315' src='" + videoId + "' allowfullscreen></iframe>";
}
})
})
})
(function() {
const thumbnailsList = document.querySelectorAll(".video-thumbnail");
window.addEventListener('DOMContentLoaded', () => {
thumbnailsList.forEach(thumbnail => {
thumbnail.addEventListener('click', () => {
if(thumbnail.hasAttribute('data-video')) {
let videoId = thumbnail.getAttribute('data-video')
thumbnail.outerHTML = "<iframe width='600' height='315' src='" + videoId + "' allowfullscreen></iframe>";
}
})
})
})
})();
  • I've also renamed the variable each to thumbnail to better represent what it is.
  • I renamed the selector from #keymanThumbnails to .video-thumbnail (a class rather than an id, and singular rather than plural, and 'video' instead of 'keyman' to clarify what it is for, and finally, using kebab-case instead of camelCase 😁)

@Meng-Heng
Copy link
Collaborator Author

Thanks @mcdurdin. I'm taking notes, this is so unfamiliar to me. 😅

@Meng-Heng Meng-Heng requested a review from mcdurdin December 9, 2024 05:48
Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

LGTM!

@Meng-Heng Meng-Heng requested review from darcywong00 and removed request for darcywong00 December 9, 2024 07:01
@darcywong00 darcywong00 merged commit 57fce21 into keymanapp:master Dec 11, 2024
3 checks passed
@Meng-Heng Meng-Heng deleted the keyman-videos-website branch December 12, 2024 05:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

feat: change keyman videos from link to video showcasing

3 participants