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

fixing hover effect on cards on News page #4691

Merged
merged 4 commits into from
Aug 23, 2023

Conversation

VamshiReddy02
Copy link
Contributor

Description

This PR fixes #4470

I'm sorry for closing my Last PR because of some technical issues. As you all suggested I added Margin to the images and reduced their size also, please let me know of any changes.

Signed commits

  • Yes, I signed my commits.

@l5io
Copy link
Contributor

l5io commented Aug 4, 2023

🚀 Preview for commit 34515fa at: https://64ccac8c0ad7296318b84040--layer5.netlify.app

@Ghat0tkach
Copy link
Member

Hey @VamshiReddy02

Let's discuss it on the websites call.
Please add this as an agenda item in the meeting minutes, if you would :)

https://docs.google.com/document/d/1XczAHXVe2FIWPqiF57ospJ43zw5cZQ7ui8mn39v5EvA/edit#heading=h.lohhtewfwima

Copy link
Member

@hirentimbadiya hirentimbadiya left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Contributor

@abhijeetgauravm abhijeetgauravm left a comment

Choose a reason for hiding this comment

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

image
At this card here on hovering , still image is cutting! @VamshiReddy02

Copy link
Contributor

@HetviSoni HetviSoni left a comment

Choose a reason for hiding this comment

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

+1 to @abhijeetgauravm 's point and also one more. The bottom of this image is getting cut even before we hover
image

@l5io
Copy link
Contributor

l5io commented Aug 10, 2023

🚀 Preview for commit ecbd9f7 at: https://64d516ba8eac69031382dbd6--layer5.netlify.app

@VamshiReddy02
Copy link
Contributor Author

Hey @abhijeetgauravm and @HetviSoni, I made some changes. Please Review my changes again and please let me know of any changes.

height:80%;
width:70%;
margin-top: 0.5rem;
margin-left: 3rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you have explicitly added margin-left as 3rem which is causing alignment issues on some screen size between desktop and mobile(image is not in center of the card).

Maybe you can try
margin:auto; margin-top:0.5rem;
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Well good work @VamshiReddy02 it is working for the desktop screen.

Signed-off-by: Vamshi Reddy <[email protected]>
@l5io
Copy link
Contributor

l5io commented Aug 10, 2023

🚀 Preview for commit 2bd692f at: https://64d529b3903428009dc8d570--layer5.netlify.app

@VamshiReddy02
Copy link
Contributor Author

Thank you @HetviSoni for the suggestion. I added margin: auto; into the code, please check it out.

Copy link
Contributor

@HetviSoni HetviSoni left a comment

Choose a reason for hiding this comment

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

Good work @VamshiReddy02 😄

Copy link
Contributor

@abhijeetgauravm abhijeetgauravm left a comment

Choose a reason for hiding this comment

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

Looks Good @VamshiReddy02

@VamshiReddy02
Copy link
Contributor Author

Hey Everyone, If everything looks good then can you merge this PR?

Copy link
Contributor

@thisiskaransgit thisiskaransgit left a comment

Choose a reason for hiding this comment

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

@VamshiReddy02 Not quite,

  • too much padding in directions where it was not needed
  • updating classes' styles that are assigned to multiple elements will have effects, go to /resources, and /blog sections to review the changes.

We needed to specifically update the particular section's elements. Since the images are of different dimensions, we cannot ensure to prevent image overflow for each of them.

@VamshiReddy02 @HetviSoni, please conclude this task by this week

@HetviSoni
Copy link
Contributor

Ah yes @thisiskaransgit I just forgot that it would also affect other elements😯. @VamshiReddy02 please look into these suggestions.

@l5io
Copy link
Contributor

l5io commented Aug 11, 2023

🚀 Preview for commit 99ddeb8 at: https://64d68fbcfaf31907e05eeff1--layer5.netlify.app

@HetviSoni
Copy link
Contributor

LGTM now

@vishalvivekm
Copy link
Member

@VamshiReddy02 Let's discuss it on the websites call. Please add this as an agenda item in the meeting minutes, if you would :)

@VamshiReddy02
Copy link
Contributor Author

hello everyone, please let me know of any changes.

Copy link
Member

@Chadha93 Chadha93 left a comment

Choose a reason for hiding this comment

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

@Chadha93 Chadha93 merged commit 88d09e5 into layer5io:master Aug 23, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[UI] hover effect on cards on news page
9 participants