-
Notifications
You must be signed in to change notification settings - Fork 90
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
feat: youtube videos modal #651
Conversation
Bumps [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/eslint-plugin) from 6.20.0 to 7.5.0. - [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases) - [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/CHANGELOG.md) - [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v7.5.0/packages/eslint-plugin) --- updated-dependencies: - dependency-name: "@typescript-eslint/eslint-plugin" dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]>
Sorry, accidentally added packages.json and lock to the changes
Accidentally added to changes
These two failed checks are not related to my changes, as I see it, but maybe I'm wrong |
@KirilCycle you're right, it's weird. I'll take a look, thank you 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, thank you!
I left few comments
function closeAndStop() { | ||
const iframe = iframeRef.current | ||
|
||
if (iframe) { | ||
iframe.src = iframe.src | ||
} | ||
|
||
setVisible(false) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's remove the iframe from the DOM instead of stopping the video by cleaning the src param
onCancel={() => { | ||
setVisible(false) | ||
}}> | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | |
{ visible && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello! This wont work, the Modal will roll it back to true 'visible' state, destroyOnClose={true} will destroy the entire modal after animation, i cheked it works fine
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, I see. I missed the destroyOnClose
property. thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! 👏
Hi @donno2048 , I'm not sure why it would fail. Seems like the output from your script might be missing some information to get the entire context of the failure. Would you please help us understand how it should be used? 🙏 see here: https://github.com/hasadna/open-bus-map-search/actions/runs/8602435326/job/23571996099?pr=651 |
@donno2048 I think that having the job called |
I was busy is this still relevant? |
@donno2048 seems like @KirilCycle solved this issue |
@all-contributors please add @KirilCycle for his great code contribution 👏 |
I've put up a pull request to add @KirilCycle! 🎉 |
issue #605
Description
I added info icons that open a modal with video about current page.
screenshots