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

Add and test sign language video in the container page #3194

Open
wants to merge 8 commits into
base: dev-sign
Choose a base branch
from

Conversation

seokho-son
Copy link
Collaborator

Describe your changes

This testing PR is to support #3044
Will see and discuss about the layout from the preview.

Related issue number or link (ex: resolves #issue-number)

Checklist before opening this PR (put x in the checkboxes)

  • This PR does not contain plagiarism
    • don’t copy other people’s work unless you are quoting and contributing it to them.
  • I have signed off on all commits
    • signing off (ex: git commit -s) is to affirm that commits comply DCO. If you are working locally, you could add an alias to your gitconfig by running git config --global alias.ci "commit -s".

@seokho-son seokho-son added the lang/sign for Sign language label Jun 12, 2024
@github-actions github-actions bot added the lang/en for English label Jun 12, 2024
Copy link

netlify bot commented Jun 12, 2024

Deploy Preview for cncfglossary ready!

Name Link
🔨 Latest commit ca40cbe
🔍 Latest deploy log https://app.netlify.com/sites/cncfglossary/deploys/66e03aa5bf53e400084286fe
😎 Deploy Preview https://deploy-preview-3194--cncfglossary.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@seokho-son
Copy link
Collaborator Author

@seokho-son seokho-son added the hold Wait, please do not proceed this yet label Jun 12, 2024
Copy link

Wait, please do not proceed with this yet.

Copy link

To see where this is in the review pipeline and follow the progress, please look at the definition review board.

@seokho-son
Copy link
Collaborator Author

hold

  • this PR is WIP. :)

@seokho-son
Copy link
Collaborator Author

Note: {{< youtube YOUTUBE-VIDEO-LINK-ID >}} is HUGO built-in feature. :)

content/en/container.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

I'd suggest using something like one of the following, instead of introducing new sections:

Of course the latter is graphically nicer, but would require that this repo be using the latest Docsy (and possibly wait for an accordion shortcode to land -- a PR is ready, it just hasn't been accepted yet). <summary> is immediately accessible

For example:

Sign language

[video goes here]

Also, given that the video is prominent when shown, using either a summary element or an accordion hides the video until a reader wishes to consult it. That feels like a better overall page design and general UX to me.

WDYT @nate-double-u @seokho-son et al?

@nate-double-u
Copy link
Member

## Further Reading may not be a necessary header, but I added it to separate the In sign language section from the content above.

I agree. If we're going to roll out a new template with ## Further Reading as a header, I feel like we should specify what it's for. Otherwise, it may become a bit of a junk drawer, which would undermine the effort we're making to highlight the sign language section.

Let us consider dropping "Further reading" in favour of @chalin's suggestion of a "Sign language" section (how ever we want to render that).

Signed-off-by: Catherine Paganini <[email protected]>
Copy link
Collaborator

@CathPag CathPag left a comment

Choose a reason for hiding this comment

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

I still have maintainer rights, so I went ahead and made the H2 updated 😜

@jtjackson
Copy link

LGTM 😄

@seokho-son
Copy link
Collaborator Author

Hello @chalin @nate-double-u,
Thank you for sharing your thoughts. The reason I did not use <summary> or another style in the initial proposal was that I hoped the sign language videos would become visible to people more quickly. (It was also the initial layout proposal from the glossary-sign-language and deaf-and-hard-of-hearing WG)
For now, I've uploaded a version that incorporates both of your feedback for testing on this page.
It would be great if we could all review it together and align our thoughts.

https://deploy-preview-3194--cncfglossary.netlify.app/container/

  • Using <summary> to hide/open the video content:

    • Pros: Emphasizes the main content of the glossary.
    • Cons: Requires an additional click to access the sign language video. (It might not be immediately apparent on the page that the glossary has started supporting sign language)
  • Providing it at the end under an H2 or H3 header:

    • Pros: The opposite of the above method.
    • Cons: The opposite of the above method.

I would also appreciate the opinions of others like @CathPag @jtjackson.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Thanks for showing both alternatives.

I'd be ok either way. What do the WG members have to say about it?

My suggestion would be to create a Hugo shortcode that accepts an YouTube video ID as argument, and then displays whichever solution is adopted here (summary or heading).

I get the impression that there is a preferences for a regular H2 heading at the end of the page. As I mentioned, that works for me. The advantage of using a shortcode is that if we change our mind later, it will be easy to adjust across the board.

WDYT?

Also see inline suggested adjustments.

content/en/container.md Outdated Show resolved Hide resolved
content/en/container.md Outdated Show resolved Hide resolved
content/en/container.md Outdated Show resolved Hide resolved
iamNoah1 and others added 3 commits September 10, 2024 13:24
Co-authored-by: Patrice Chalin <[email protected]>
Signed-off-by: Noah Ispas <[email protected]>
Co-authored-by: Patrice Chalin <[email protected]>
Signed-off-by: Noah Ispas <[email protected]>
Co-authored-by: Patrice Chalin <[email protected]>
Signed-off-by: Noah Ispas <[email protected]>
@iamNoah1
Copy link
Collaborator

I prefer the second option. What do we need to proceed here? @seokho-son @nate-double-u @CathPag

@Deafveloper
Copy link
Contributor

I prefer the second option. What do we need to proceed here? @seokho-son @nate-double-u @CathPag

I would like to be notified as well for the next steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Wait, please do not proceed this yet lang/en for English lang/sign for Sign language
Projects
Status: 📋 Backlog
Development

Successfully merging this pull request may close these issues.

7 participants