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

docs: Add description about img in artifact-visualization.md #11732

Merged
merged 10 commits into from
Sep 6, 2023
Merged

docs: Add description about img in artifact-visualization.md #11732

merged 10 commits into from
Sep 6, 2023

Conversation

juijeong8324
Copy link
Contributor

Motivation

image
It is difficult to know that img can link to play demo.

Modifications

image
Therefore I add description of img.

Verification

.

Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

Signed-off-by: Justice <juijeong8324@gmail.com>
Signed-off-by: juijeong8324 <juijeong8324@gmail.com>

Signed-off-by: Justice <juijeong8324@gmail.com>
@terrytangyuan terrytangyuan enabled auto-merge (squash) September 3, 2023 11:38
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I didn't even know this was a video, nice change!

I am thinking though that there are better, more conventional ways of indicating that it is a video:

  1. Embed the video directly in the page
  2. Use an image with a play button in the middle (or otherwise overlay such a play button)

docs/artifact-visualization.md Outdated Show resolved Hide resolved
@agilgur5 agilgur5 added the area/docs Incorrect, missing, or mistakes in docs label Sep 4, 2023
Co-authored-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
Signed-off-by: Justice <juijeong8324@gmail.com>
auto-merge was automatically disabled September 4, 2023 04:36

Head branch was pushed to by a user without write access

@juijeong8324 juijeong8324 reopened this Sep 4, 2023
GeunSam2 and others added 3 commits September 4, 2023 04:59
Signed-off-by: GeunSam2 <rootiron96@gmail.com>
Signed-off-by: GeunSam2 <rootiron96@gmail.com>
…juijeong8324

Fix/docs lint error for juijeong8324
@juijeong8324
Copy link
Contributor Author

juijeong8324 commented Sep 4, 2023

I didn't even know this was a video, nice change!

I am thinking though that there are better, more conventional ways of indicating that it is a video:

  1. Embed the video directly in the page
  2. Use an image with a play button in the middle (or otherwise overlay such a play button)

@GeunSam2 suggests to me as follows...
I contemplated the idea of generating and incorporating a distinct image, but it seemed challenging to upload a new image to a public repository and guarantee its credibility. Consequently, I opted to add a caption at the bottom of the image!

Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Consequently, I opted to add a caption at the bottom of the image!

This is still an improvement over what it was before, so I'm ok with this.

For other readers/reviewers, see the rendered version

@agilgur5
Copy link
Member

agilgur5 commented Sep 4, 2023

I contemplated the idea of generating and incorporating a distinct image, but it seemed challenging to upload a new image to a public repository and guarantee its credibility.

The docs/assets/ directory has many images already, so one could be added there.

Add artifact-visualization-demo.png at docs/assets

Signed-off-by: Justice <juijeong8324@gmail.com>
Signed-off-by: Justice <juijeong8324@gmail.com>
Copy link
Member

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

Nice! LGTM, thanks for iterating on this!

@terrytangyuan terrytangyuan merged commit 94fbb3b into argoproj:master Sep 6, 2023
24 checks passed
qudtjs0753 pushed a commit to qudtjs0753/argo-workflows that referenced this pull request Sep 6, 2023
@juijeong8324
Copy link
Contributor Author

Actually I have tried to use <iframe> tag to embed the video directly, it can't apply.
However I find out that <iframe> tag is not supported in Github Markdown. (Disallowed Raw HTML (extension) or Stack Overflow) If you know how to embed the video in Github markdown, please let me know.

@agilgur5
Copy link
Member

agilgur5 commented Sep 9, 2023

Yea it's not allowed in GH Markdown (no interactive elements basically), but I would think it would work on the rendered docs site. Did it not work on the docs site?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/docs Incorrect, missing, or mistakes in docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants