-
Notifications
You must be signed in to change notification settings - Fork 802
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 testimonial section on homepage #3527
Add testimonial section on homepage #3527
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo UI looks good to me but I don't think we should put a dummy testimonial to the staging server.
\cc: @Ram81
@@ -429,8 +429,30 @@ <h3 class="fw-light center">Sponsors</h3> | |||
</div> | |||
</div> | |||
</section> | |||
<!-- Subscribe to newsletter --> | |||
<section class="ev-container text-med-black ev-super-light-bg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Css seems a bit distorted for mobile devices. Can you please try making padding:0px
for .grad-container
just for the testimonial container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The left and right buttons? Because I had seen the mobile view as well. I will check it once again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kajol-Kumari Currently, the mobile view is:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we can use the left and right padding spaces just for mobile devices. It would look much cleaner that way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kajol-Kumari Got it. Done the required changes.
@@ -429,8 +429,30 @@ <h3 class="fw-light center">Sponsors</h3> | |||
</div> | |||
</div> | |||
</section> | |||
<!-- Subscribe to newsletter --> | |||
<section class="ev-container text-med-black ev-super-light-bg"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @gautamjajoo Instead of renaming grad-container
to grad-conatiner-testimonial
and repeating the styles of .grad-container
at here You can instead put an extra wrapper class eg testimonial-wrapper
here like <section class="ev-container testimonial-wrapper text-med-black ev-super-light-bg">
and just just do
.testimonial-wrapper {
.grad-container {
padding:0px;
}
}
under the media query for mobile devices
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@RishabhJain2018 do we also want to show the challenge host team logo in testimonials? All testimonials are from host teams right? |
Yes, that would be great! |
@RishabhJain2018 @Ram81 Should the logos be on the top of the card? |
We can keep it just before the name. Instead of ref - here |
Yes, I like the above design. We should make our testimonials card similar. |
@Ram81 @Kajol-Kumari I have attached images for both the ways, is the first one similar to what we expect in above design? |
Hey @gautamjajoo Can we please try to keep the logo in circular format and that also in small circular of maybe 400*400 px? As currently, logo is acquiring a larger section which will not look good for other logos having dark backgrounds. Please see the above reference image |
@gautamjajoo can we show the logo and name on the left column and testimonial in the right column. Both should be in same row on the testimonial card. |
@Ram81 Like this? Actually the name would move either below the pic or below the text. |
@gautamjajoo yes, this looks better |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo can you resolve merge conflicts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gautamjajoo minor comments. Rest of the PR LGTM 👍
@RishabhJain2018 can you take a look |
yeah, the screenshot looks good to me! |
@Ram81 Updated the PR! |
* add testimonial section on homepage * removed unnecessary animations * remove padding for mobile view * add wrapper * add testimonial org and logo * fix the testimonial card * fix indentation Co-authored-by: Rishabh Jain <[email protected]>
Added testimonial section on homepage and removed the
testimonial
component as well.The current data can be replaced with the real data here.
@Ram81 @Kajol-Kumari