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 optional link to video page for video blocks #926

Merged
merged 8 commits into from
Sep 12, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Aug 25, 2023

closes #854

This adds an optional link to video blocks which links directly to the video page of that block.
Right now it is a link button but I wouldn't mind if this were just a link (just text + icon without the background and outline).

@github-actions github-actions bot temporarily deployed to test-deployment-pr926 August 25, 2023 14:10 Destroyed
@dagraf
Copy link
Collaborator

dagraf commented Aug 30, 2023

The momentarily implemented solution is quite confusing. Users might ask themselves what this "Video page" might be and what the option "Show link" is actually doing.

Suggestion:

  • Rename option to: "Show sharing option"
  • If activated: Offer a button "Share" with similar behaviour to the "Share" button on video pages.
  • Clicking on this button "Share": A modal gets opened offering a link that can get copy-pasted and it is explained that this is a direct link to the video without the integration into the current page.

@owi92
Copy link
Member Author

owi92 commented Aug 30, 2023

Thank you for your feedback.
To be honest, I dont really understand how the solution presented here is confusing (apart from the wording on the checkbox, which should indeed clarify that it will show a link to the video page). I don't mind your new suggestion, but reading your and @oas777's comments on #854 gave me the impression that both of you agreed with @LukasKalbertodt's suggestion to "include a small link below the video saying "go to video page ->", with the link doing basically the same as clicking a video inside series blocks.

Edit: I have updated the wording and changed the link from a "direct link" to one that links to the video "in context" (the same link that is shown in the regular share menu - I think that was one of the issues you mentioned in our meeting? Though in the above comment you suggest using a direct link, so I'm not sure on that). Maybe that makes this less confusing. I am still open for your suggestion but I would like to wait for Lukas' and Olaf's opinions on this.

@oas777
Copy link
Collaborator

oas777 commented Aug 30, 2023

After revisiting #854, I'm with Ole on this one (and I wasn't fully aware what I was looking at during our meeting, apologies) who did what the ticket asked for. The idea of "sharing" wasn't part of that - and can happen on the "video page". When I'm embedding a video on a webpage, I don't necessarily have sharing in mind, the player usually does offer that option (not Paella, I just realize).

So if sharing is on your mind regardless, David, the link to the "video page" should be replaced by the "Share" symbol (activated by the user with a "Allow sharing" click when embedding the video).

@github-actions github-actions bot temporarily deployed to test-deployment-pr926 August 30, 2023 17:51 Destroyed
@owi92 owi92 added the changelog:user User facing changes label Sep 1, 2023
@dagraf
Copy link
Collaborator

dagraf commented Sep 4, 2023

After reviewing @owi92's changes:

  • I appreciate that you have changed the link to "video in context". I think, this makes it less confusing. Therefore, forget about the suggestion I made in my last comment.
  • What do you think about changing the wording of the option (to "Show button 'To video page'") and button (to "To video page"). Reason: That's what this option does, it offers a button that sends me to the video page when I click on it - and it does not show the link.

@oas777
Copy link
Collaborator

oas777 commented Sep 5, 2023

David, I think you're overthinking this one... again. Your suggestion is like renaming the "Home" button to "Take me to the homepage please". I'm fine with the current solution.

@dagraf
Copy link
Collaborator

dagraf commented Sep 5, 2023

That's why I asked for your opinions. I can live with the naming of the button. My concern is, that the option I check does not exactly what it says. It does not "show me the link to the video page", it shows a button which takes me to the video page when clicking on it. I would therefore still suggest to rename the option to "Show button 'To video page'".

@owi92
Copy link
Member Author

owi92 commented Sep 6, 2023

I'm fine with David's naming suggestion and changed the wording accordingly. Please note that it will be a while until this gets deployed since we are dealing with some server issues at the moment (that's why the check below is failing).

@oas777
Copy link
Collaborator

oas777 commented Sep 6, 2023

Well, now I know where you're standing, Ole...

@owi92 owi92 changed the title Page link for video blocks Add optional link to video page for video blocks Sep 11, 2023
@github-actions github-actions bot temporarily deployed to test-deployment-pr926 September 11, 2023 12:30 Destroyed
@LukasKalbertodt
Copy link
Member

I think the label "Go to video page" is fine. Not using the direct link, but the "in page link" is a good call as well, IMO.

Design wise, I'm not super satisfied. What do you all think about aligning it to the right of the video:

image

Or even removing the button look and just going with a normal link?

image

(I didn't put much thought into how the link looks, I'm sure this can still be improved)

Not using a button would have the advantage of not disrupting the page flow as much. (Imagine lots of text on a page with the occasional video in between.) It's still something users are able to find, but if this link is too intrusive, then I fear many will disable it because it "makes the page look ugly".

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Nice work, especially working on the backend alone! I only have some rather minor things, I think.

backend/src/db/migrations/23-video-block-show-link.sql Outdated Show resolved Hide resolved
frontend/src/i18n/locales/en.yaml Outdated Show resolved Hide resolved
frontend/src/routes/manage/Realm/Content/AddButtons.tsx Outdated Show resolved Hide resolved
@dagraf
Copy link
Collaborator

dagraf commented Sep 11, 2023

Or even removing the button look and just going with a normal link?

image

I'm in favor of this solution (right align and going with a normal link).

@github-actions github-actions bot temporarily deployed to test-deployment-pr926 September 11, 2023 16:19 Destroyed
@oas777
Copy link
Collaborator

oas777 commented Sep 11, 2023

I love to be that guy, so if you're saying it's a "normal link", why the arrow? Other (normal) links are blue and underlined, no arrows.

@oas777
Copy link
Collaborator

oas777 commented Sep 11, 2023

PS: "Click here to go to the video page and in case this isn't clear there's an additional arrow you can click on."

Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Good changes, just three tiny things.

backend/src/db/migrations/23-video-block-show-link.sql Outdated Show resolved Hide resolved
backend/src/db/migrations/23-video-block-show-link.sql Outdated Show resolved Hide resolved
frontend/src/ui/Blocks/Video.tsx Outdated Show resolved Hide resolved
@LukasKalbertodt
Copy link
Member

@oas777 Oh, somehow I missed your comments until now. Fair point, but I don't personally mind the arrow. I think it works better with the arrow than without.

@oas777
Copy link
Collaborator

oas777 commented Sep 12, 2023

Good morning! "it works better with the arrow than without" - what does that mean? Is it more reliable? Faster? Jokes aside, this is about design and coherence, so I don't think the arrow should be there, unless we're using it somewhere else already.

@LukasKalbertodt
Copy link
Member

Ja moin! By "works better" I just meant: looks better and immediately suggests what it does. I feel like the right-arrow (implying a rightwards motion) is very intuitive. I also don't think it breaks coherence, I don't find it very disruptive. And yes, we indeed use it elsewhere already:

image

But ok, here a comparison for reference:

image

image

Again, I'm in favor of keeping the arrow.

@dagraf
Copy link
Collaborator

dagraf commented Sep 12, 2023

I am also in favor of keeping the arrow.

I don't think this was particularly necessary or useful, and it even
results in more code than before, but oh well I guess it doesn't hurt either.
This way the checkbox component is at least reusable...
@github-actions github-actions bot temporarily deployed to test-deployment-pr926 September 12, 2023 08:26 Destroyed
@LukasKalbertodt
Copy link
Member

In interest of getting PRs merged, I will merge this now. If you, Olaf, are still strongly against the arrow, it can still be removed later.

@LukasKalbertodt LukasKalbertodt merged commit 908632a into elan-ev:master Sep 12, 2023
3 checks passed
owi92 pushed a commit to owi92/tobira that referenced this pull request Sep 12, 2023
closes elan-ev#854

This adds an optional link to video blocks which links directly to the
video page of that block.
Right now it is a link button but I wouldn't mind if this were just a
link (just text + icon without the background and outline).
@oas777
Copy link
Collaborator

oas777 commented Sep 12, 2023

That's two in favor and I know where Ole stands (#926 (comment)), so go ahead and keep ignoring me...

@oas777
Copy link
Collaborator

oas777 commented Sep 12, 2023

PS: It actually looks better with the arrow, true.

@owi92 owi92 deleted the page-link-for-video-blocks branch March 4, 2024 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show more metadata or link to video page on video blocks
4 participants