meshery / play

Cloud Native Playground for Kubernetes and all CNCF projects
https://play.meshery.io
Apache License 2.0
91 stars 90 forks source link

fixed react player alt image and fixed padding for caption #76

Closed vishnus17 closed 1 year ago

vishnus17 commented 1 year ago

Notes for Reviewers

This PR fixes #75

Signed commits

netlify[bot] commented 1 year ago

Deploy Preview for meshery-play ready!

Name Link
Latest commit 58a59db094a54dde6762265acda6c694ef4e0bfe
Latest deploy log https://app.netlify.com/sites/meshery-play/deploys/6417f0f2221188000869a9d9
Deploy Preview https://deploy-preview-76--meshery-play.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 settings.

Yashsharma1911 commented 1 year ago

nice work @vishnus17 as you are there can you do a small change, as you can see the corner of the player in the below image is not in a proper curve can you fix this too

Screenshot 2023-02-25 at 3 24 27 AM

vishnus17 commented 1 year ago

@Yashsharma1911 . I don't think I can do that. The image fixing I did was with canva. It was just adding a rectangle over the Login button which I noticed when I was signing up for the early access. I'm not really that much of a graphics guy 😅

Nikhil-Ladha commented 1 year ago

nice work @vishnus17 as you are there can you do a small change, as you can see the corner of the player in the below image is not in a proper curve can you fix this too

Screenshot 2023-02-25 at 3 24 27 AM

Where do you see that the corner doesn't have the same radius as the image? 🤔 Seems same to me, or am I just not able to see it

Yashsharma1911 commented 1 year ago

nice work @vishnus17 as you are there can you do a small change, as you can see the corner of the player in the below image is not in a proper curve can you fix this too Screenshot 2023-02-25 at 3 24 27 AM

Where do you see that the corner doesn't have the same radius as the image? 🤔 Seems same to me, or am I just not able to see it

yes it is actually not properly curved this is because radius is given in %

current

Screenshot 2023-02-28 at 3 02 12 PM

Desired

Screenshot 2023-02-28 at 3 03 15 PM

github-actions[bot] commented 1 year ago

Checking in... it has been awhile since we've heard from you on this issue. Are you still working on it? Please let us know and please don't hesitate to contact a MeshMate or any other community member for assistance.


        Be sure to join the community, if you haven't yet and please leave a :star: star on the project :smile:

leecalcote commented 1 year ago

@vishnus17 following up a final time here... is this something that you intend to complete?

adithyaakrishna commented 1 year ago

@vishnus17 This is a pretty small change, lemme guide you through it

In this file at Line 200, https://github.com/meshery/play/blob/c84664fbd90cf126c569f0017b4616a77638f9c1/site/src/App.style.js#L200, change the value from 2.5% to 1.5%, or you can change it to 15px also (if you dont want to use the % values), after this you should get your desired status :)

Lmk if you have any doubts while working on it :)

@Nikhil-Ladha Also the issue seems to be arising from the background, there's like two separate colours here

Screenshot 2023-03-13 at 10 33 55 PM

and on changing the colour theme from dark to light, you can see a small difference in the background 👀

CC @Yashsharma1911 @leecalcote

adithyaakrishna commented 1 year ago

@vishnus17 The fix should be to change the value from 2.5% to 15px, that should fix the case of two backgrounds being visible at the same time as well :)

vishnus17 commented 1 year ago

Hi guys. Sorry I was having really busy weeks lately. @leecalcote . Yes I'll complete this. @adithyaakrishna Thanks for the help. I'll complete this by this week.

leecalcote commented 1 year ago

Sounds great.

vishnus17 commented 1 year ago

Hi @Yashsharma1911, @Nikhil-Ladha , @leecalcote , @adithyaakrishna . I've removed the padding for the caption element and adjusted the border-radius of the react player image. Please have a final look and see if its good to be merged. Thanks.

adithyaakrishna commented 1 year ago

@Nikhil-Ladha Could you please merge this PR?