refactor: Remove unused Alert component and update video title in Tim…#794
refactor: Remove unused Alert component and update video title in Tim…#794
Conversation
NoamGaash
left a comment
NoamGaash
left a comment
There was a problem hiding this comment.
Thanks for contributing to open-bus!
is this PR ready for review?
| <InfoYoutubeModal | ||
| label={t('open_video_about_this_page')} | ||
| title={t('gaps_page_description')} | ||
| videoUrl="" |
There was a problem hiding this comment.
It is empty because as far as I have seen there is no suitable video to put in place. I thought that maybe down the road someone could produce a suitable video.
If you have another idea I would be happy, or even better if there is a suitable video that I missed.
I also thought of adding it so that it was clear, write me if you want to add it
the PR is ready for review.
There was a problem hiding this comment.
thanks for the clarification!
so I guess we should / could remove the iframe if there's no video to be shown (?)
There was a problem hiding this comment.
I don't want to destroy the element
And thank you very much for the feedback
There was a problem hiding this comment.
what do you mean by destring the element?
I meant to say that if the modal component gets no url, it should not render an iframe inside of it
|
side note - https://eyes.applitools.com/app/test-results/00000251684207444242/?accountId=ClQJqzT0PkebrsewHfaQEQ__ |
|
I did not think of that I will do that
בתאריך יום ד׳, 12 ביוני 2024, 14:38, מאת Noam Gaash <
***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In src/pages/gaps/index.tsx
<#794 (comment)>
:
> {t('gaps_page_title')}
+ <InfoYoutubeModal
+ label={t('open_video_about_this_page')}
+ title={t('gaps_page_description')}
+ videoUrl=""
what do you mean by destring the element?
I meant to say that if the modal component gets no url, it should not
render an iframe inside of it
—
Reply to this email directly, view it on GitHub
<#794 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AX35RNMMWOHC33EKGDXND4LZHAXJRAVCNFSM6AAAAABJDXSLROVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMJSG43DQNZYGU>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
2 participants
…eBasedMapPage
I did this as a draft because I want to make all the headlines of all pages before making a change to leave the uniformity of the site
Description
Move the page's explanation into the model to clean the page a bit
Even the pages that have no video can still be done and maximum down the road will make them a video but I think you can definitely make a infrastructure on the page for a video