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

feat: youtube videos modal #651

Merged
merged 23 commits into from
Apr 8, 2024
Merged

feat: youtube videos modal #651

merged 23 commits into from
Apr 8, 2024

Conversation

KirilCycle
Copy link
Collaborator

@KirilCycle KirilCycle commented Apr 6, 2024

issue #605

Description

I added info icons that open a modal with video about current page.

  • create a youtube info component that will open a modal with a youtube video
  • add this component to: 1) map pages 2) dashboard page 3) gap patterns page 4) bug report form
  • link to relevant videos in out readme and contribution files

screenshots

Screenshot 2024-04-06 at 11 18 37 (2)
Screenshot 2024-04-06 at 11 18 58
Screenshot 2024-04-06 at 11 19 31
Screenshot 2024-04-06 at 11 25 56
Screenshot 2024-04-06 at 11 19 11

@KirilCycle KirilCycle marked this pull request as draft April 6, 2024 08:38
Sorry, accidentally added packages.json and lock to the changes
Accidentally added to changes
@KirilCycle KirilCycle changed the title Feat/youtube videos modal feat:youtube videos modal Apr 6, 2024
@KirilCycle KirilCycle changed the title feat:youtube videos modal feat: youtube videos modal Apr 6, 2024
@KirilCycle
Copy link
Collaborator Author

KirilCycle commented Apr 6, 2024

These two failed checks are not related to my changes, as I see it, but maybe I'm wrong

@KirilCycle KirilCycle marked this pull request as ready for review April 6, 2024 09:28
@NoamGaash
Copy link
Member

@KirilCycle you're right, it's weird. I'll take a look, thank you 🙏
the screenshots looks perfect 👏

Copy link
Member

@NoamGaash NoamGaash left a 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

README.md Outdated Show resolved Hide resolved
src/locale/he.json Outdated Show resolved Hide resolved
src/locale/en.json Outdated Show resolved Hide resolved
src/pages/BugReportForm .tsx Outdated Show resolved Hide resolved
Comment on lines 16 to 24
function closeAndStop() {
const iframe = iframeRef.current

if (iframe) {
iframe.src = iframe.src
}

setVisible(false)
}
Copy link
Member

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

src/pages/components/YoutubeModal.tsx Outdated Show resolved Hide resolved
@KirilCycle KirilCycle marked this pull request as draft April 6, 2024 15:12
@KirilCycle KirilCycle marked this pull request as ready for review April 6, 2024 15:27
onCancel={() => {
setVisible(false)
}}>
{
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
{ visible &&

Copy link
Collaborator Author

@KirilCycle KirilCycle Apr 7, 2024

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

Copy link
Member

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!

Copy link
Member

@NoamGaash NoamGaash left a comment

Choose a reason for hiding this comment

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

Great work! 👏

@NoamGaash
Copy link
Member

NoamGaash commented Apr 8, 2024

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
for commit 3f25da2

@NoamGaash NoamGaash linked an issue Apr 8, 2024 that may be closed by this pull request
3 tasks
@NoamGaash NoamGaash enabled auto-merge (squash) April 8, 2024 17:02
@NoamGaash
Copy link
Member

@donno2048 I think that having the job called validate/lint would provide better DX
image

@NoamGaash NoamGaash merged commit 390fe95 into main Apr 8, 2024
16 checks passed
@NoamGaash NoamGaash deleted the feat/youtube-videos-modal branch April 8, 2024 17:06
@donno2048
Copy link
Collaborator

I was busy is this still relevant?

@NoamGaash
Copy link
Member

@donno2048 seems like @KirilCycle solved this issue
If you see we've missed a video, or there is any improvement you want to make, we're open to that

@NoamGaash
Copy link
Member

@all-contributors please add @KirilCycle for his great code contribution 👏

Copy link
Contributor

@NoamGaash

I've put up a pull request to add @KirilCycle! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add youtube videos
3 participants