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 cloud video as iframe #25

Merged
merged 4 commits into from Nov 5, 2019
Merged

Conversation

@sukhbeersingh
Copy link
Contributor

sukhbeersingh commented Oct 27, 2019

What has been changed
Added iframe element to the top of nowcasting page.

Why was the change necessary
Fix issue #14

Please let me know if the position looks good. Putting it on the top provides instantaneous visibility.

@sukhbeersingh sukhbeersingh requested a review from FWirtz as a code owner Oct 27, 2019
@JackKelly

This comment has been minimized.

Copy link

JackKelly commented Oct 29, 2019

Hi @sukhbeersingh, thanks loads for doing this!

If you wouldn't mind, please could you swap the video for this new version: https://www.youtube.com/watch?v=IOp-tj-IJpk

(The PV data and satellite data are out-of-sync by an hour in the old video!)

@FWirtz

This comment has been minimized.

Copy link
Contributor

FWirtz commented Oct 30, 2019

Awesome! Thanks for picking it up. I'll aim to review and merge your pr this evening so it's still within hacktoberfest. :-)

Copy link
Contributor

FWirtz left a comment

Thanks! Looks great on desktop!

It kinda breaks the layout for me though on mobile. 😢
If I remember correctly, responsive iframes are super annoying.
We use tailwind for everything, just found this plugin, maybe it's best to just use that if you can't think of an easy workaround for this either.

image

Lmk what you think about that.

Thanks again!

@sukhbeersingh

This comment has been minimized.

Copy link
Contributor Author

sukhbeersingh commented Oct 31, 2019

Hi @FWirtz thanks for your feedback. I put the iframe in a div and added some tailwind style css.
Thanks

Copy link
Contributor

FWirtz left a comment

Way better already, thanks!

Can we make it a little higher so that it's more like 16:9 dimensions?
It feels a little squashed in there imo.

Thanks @sukhbeersingh and sorry for being annoying with my review comments... 🙈

src/components/layout.css Outdated Show resolved Hide resolved
src/pages/projects/nowcasting.jsx Outdated Show resolved Hide resolved
@sukhbeersingh

This comment has been minimized.

Copy link
Contributor Author

sukhbeersingh commented Nov 1, 2019

Can we make it a little higher so that it's more like 16:9 dimensions?
It feels a little squashed in there imo.

I don't quite know how to. Can you please guide me how to do it? Just to confirm are you talking about the video div itself, or the blackspace inside the video?

Copy link
Contributor

FWirtz left a comment

Did some further investigating into this. I meant the video element itself, not the black bars.

I found this website and basically copied the code from there.
The explicit height and width attributes on the iframe apparently aren't needed.

I made the copied suggestions to your PR, it should now nicely scale responsively and also have the proper 16:9 feel to it. What do you think?

src/components/layout.css Outdated Show resolved Hide resolved
src/components/layout.css Outdated Show resolved Hide resolved
src/components/layout.css Outdated Show resolved Hide resolved
src/pages/projects/nowcasting.jsx Outdated Show resolved Hide resolved
src/pages/projects/nowcasting.jsx Outdated Show resolved Hide resolved
@FWirtz
FWirtz approved these changes Nov 5, 2019
Copy link
Contributor

FWirtz left a comment

Thanks!!

@FWirtz

This comment has been minimized.

Copy link
Contributor

FWirtz commented Nov 5, 2019

@all-contributors please add @sukhbeersingh for code

@allcontributors

This comment has been minimized.

Copy link
Contributor

allcontributors bot commented Nov 5, 2019

@FWirtz

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

@FWirtz FWirtz merged commit 2a50115 into openclimatefix:master Nov 5, 2019
1 check passed
1 check passed
stickler-ci No lint errors found
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.