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

fix: redirection Link fix #467

Merged
merged 11 commits into from
Dec 17, 2024
Merged

fix: redirection Link fix #467

merged 11 commits into from
Dec 17, 2024

Conversation

TenzDelek
Copy link
Contributor

Description
fixed the youtube redirection link
Related issue(s)

Fixes #466

Copy link

netlify bot commented Nov 13, 2024

Deploy Preview for peaceful-ramanujan-288045 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit d07ccfe
🔍 Latest deploy log https://app.netlify.com/sites/peaceful-ramanujan-288045/deploys/67612a6df9365b0008e64364
😎 Deploy Preview https://deploy-preview-467--peaceful-ramanujan-288045.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.

Copy link
Member

@AceTheCreator AceTheCreator left a comment

Choose a reason for hiding this comment

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

@TenzDelek, i noticed were the link is been referenced is not a button but a text, which can easily be missed. since the text is a link, then i believed it should be underlined. wyt @ashmit-coder?

@TenzDelek
Copy link
Contributor Author

@TenzDelek, i noticed were the link is been referenced is not a button but a text, which can easily be missed. since the text is a link, then i believed it should be underlined. wyt @ashmit-coder?

agree on that, currently, the underline is kept on hover. should I go on with the changes to add a underline?

@ashmit-coder
Copy link
Collaborator

@TenzDelek, i noticed were the link is been referenced is not a button but a text, which can easily be missed. since the text is a link, then i believed it should be underlined. wyt @ashmit-coder?

I dont think so @AceTheCreator , the underline seems to not go well with the design of the card, hence I dont think we should add it. The hover effect looks better and the text points towards it being a link.

Copy link
Collaborator

@ashmit-coder ashmit-coder left a comment

Choose a reason for hiding this comment

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

LGTM!

@thulieblack
Copy link
Member

Please link the text to the YouTube playlist https://youtube.com/playlist?list=PLbi1gRlP7pijItMBmw9SeeyWxuEa3jLR2&si=MNNU0r-aCmuWFPLI

@TenzDelek
Copy link
Contributor Author

Please link the text to the YouTube playlist https://youtube.com/playlist?list=PLbi1gRlP7pijItMBmw9SeeyWxuEa3jLR2&si=MNNU0r-aCmuWFPLI

done

@AceTheCreator
Copy link
Member

@TenzDelek, i noticed were the link is been referenced is not a button but a text, which can easily be missed. since the text is a link, then i believed it should be underlined. wyt @ashmit-coder?

I dont think so @AceTheCreator , the underline seems to not go well with the design of the card, hence I dont think we should add it. The hover effect looks better and the text points towards it being a link.

Hmm... I actually missed that it was a link. Why not make it a button if adding an underline is gonna mess with the UI.

wyt @Mayaleeeee?

@Mayaleeeee
Copy link
Member

@TenzDelek, i noticed were the link is been referenced is not a button but a text, which can easily be missed. since the text is a link, then i believed it should be underlined. wyt @ashmit-coder?

I dont think so @AceTheCreator , the underline seems to not go well with the design of the card, hence I dont think we should add it. The hover effect looks better and the text points towards it being a link.

Hmm... I actually missed that it was a link. Why not make it a button if adding an underline is gonna mess with the UI.

wyt @Mayaleeeee?

Thanks for the ping @AceTheCreator

Could you please provide a screenshot of where I can find the issue on the website? @TenzDelek

@TenzDelek
Copy link
Contributor Author

image

https://conference.asyncapi.com/venue/Online

cc @Mayaleeeee

Copy link
Member

@Mayaleeeee Mayaleeeee left a comment

Choose a reason for hiding this comment

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

The text (that direct users to the YouTube recording) should be in a different colour (a bright colour because of the overlay background) and underlined.

cc @TenzDelek @AceTheCreator @ashmit-coder

I think we can also use a button —since the online conference is different from the in-person ones.

Copy link
Member

@Mayaleeeee Mayaleeeee left a comment

Choose a reason for hiding this comment

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

The text (that direct users to the YouTube recording) should be in a different colour (a bright colour because of the overlay background) and underlined.

cc @TenzDelek @AceTheCreator @ashmit-coder

I think we can also use a button —since the online conference is different from the in-person ones.

@TenzDelek
Copy link
Contributor Author

image have underlined the link as maya suggested. imo only underlining the text would be best. as it is an external link a button doesn't sound right . @Mayaleeeee cc @AceTheCreator @ashmit-coder

Copy link
Member

@AceTheCreator AceTheCreator left a comment

Choose a reason for hiding this comment

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

LGTM!

@TenzDelek
Copy link
Contributor Author

any changes are still needed here or are we ready to merge 🤔

@AceTheCreator
Copy link
Member

@TenzDelek can you update this branch, so we can merge this PR?

@TenzDelek
Copy link
Contributor Author

@TenzDelek can you update this branch, so we can merge this PR?

done cc @AceTheCreator

Copy link
Member

@AceTheCreator AceTheCreator left a comment

Choose a reason for hiding this comment

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

LGTM!

@AceTheCreator
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 3f3d293 into asyncapi:master Dec 17, 2024
14 checks passed
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.

[BUG] Online Conference redirection on invalid link (youtube)
6 participants