-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
implemented the new case study section of the home page #6032
Conversation
✅ Deploy Preview for knative ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Hey @zainabhusain227 @mmejia02 when you have some time can you compare the preview to the designs and leave any feedback here? |
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 @ShravaniAK , feedback on these changes from todays UX WG review was:
- There should be a larger padding at the top of the case study boxes so that there is a bit more space between the company logo and the border of the box
- There should be more space between the "Knative is a Cloud Native Computing Foundation incubation project" text and the "Case Studies" section
- For the hover effect on the individual case studies, we should only change the border colour, and not do any box shadow. We should also add a transition to this so that it animates nicely
- The entire case study box should use the pointer for the mouse, and should be clickable and take users to the case study page. Currently, this only works if users click on the text in the box.
- We should incorporate the arrows from the design in Improvements to Case Studies Section ux#146
Thanks for all your work on this so far, once we have these fixes this should be good to merge
@Cali0707 , I have implemented all the changes requested, except for the "incorporate the arrows" , I think that it's a new feature implementation that can be made in a new separate pr. |
Also for point 2 , I have fixed that in #6036 |
@ShravaniAK it looks like this has some conflicts now that your other PR merged, would you mind fixing those? |
@Cali0707 I have fixed the conflicts! |
Thanks @ShravaniAK ! There are a few issues that need fixing (see video below):
Screencast.from.2024-07-18.11-45-07.mp4 |
@Cali0707 I have fixed both the issues, can you please review them once again, Thanks! |
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
/approve
Thanks @ShravaniAK !
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707, ShravaniAK The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #6031
implemented the new design of the case study section , did the following changes
Before :
After: