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

Added Case Studies page in Overview Section #540

Conversation

TamannaVerma99
Copy link
Contributor

What kind of change does this PR introduce?
Added a new page in Overview section to show the case studies published in the blog. Currently, since I don't have access to the data file that has information related to blogs published, I have created a sample file of my own named casestudies.json with the same data format as mentioned in the issue and linked it to the card component. This makes the JSON file easily replaceable with the type of data we'll be using.

Issue Number:

Does this PR introduce a breaking change?
No

Michael-Obele and others added 2 commits March 11, 2024 15:16
…son-schema-org#460)

* Standardize List Display with Card Component (json-schema-org#433)

* Addressed comments, added images, and implemented new styles

* Increased padding on the x-axis
@TamannaVerma99 TamannaVerma99 requested a review from a team as a code owner March 14, 2024 11:47
@TamannaVerma99 TamannaVerma99 changed the base branch from main to web-release-3 March 14, 2024 11:47
@benjagm
Copy link
Collaborator

benjagm commented Mar 16, 2024

@TamannaVerma99 Hi reviewed the PR and decided to push some changes to accelerate the process. The only pending thing on your side is to adjust the generic card component to make sure the logos shows properly. I like the logos much bigger with the right proportion bacause some of the logos show deformed.

You will need to rebase and fix the conflicts too.

What I fixed:

  • Modified the json data to remove posts unrelated to case studies and updated the logos.
  • Added all the logos.
  • Modified how you was using the card component to use the image property to show the logo, instead of the icon as you was using. Now the behaviour is correct.

You did an amazing job and we are very very close.
Congrats!!

Copy link
Collaborator

@benjagm benjagm left a comment

Choose a reason for hiding this comment

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

Nice job. We are very very close.

See my review comments here

@TamannaVerma99
Copy link
Contributor Author

Nice job. We are very very close.

See my review comments here

Thanks :) I will do the required changes asap.

<img src={icon} alt={title} className='h-full w-full' />
</span>
)}
<p className='mt-1 items-center text-[0.9rem] font-bold text-gray-900'>
Copy link
Contributor

@Michael-Obele Michael-Obele Mar 17, 2024

Choose a reason for hiding this comment

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

Hey there! Great work on the code! I noticed the title seemed a bit small. Might we consider text-h5 for better readability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey! Thanks for the feedback .I will work on it. Thanks:)

@Michael-Obele
Copy link
Contributor

Michael-Obele commented Mar 18, 2024

Hi everyone,
I realize now that the new card component was likely created because the long title was affecting the overall design of the component. This is something I hadn't considered initially.

I'd be happy to adjust the existing card component by adding a conditional statement. This would check the word count and automatically adjust the text size to be larger for shorter content and smaller for longer content.

To keep things moving smoothly, I can either open a separate issue for this or jump right in and make the fix here in the PR. Just let me know which approach you prefer!

@VivekJaiswal18
Copy link
Contributor

Hi everyone, I realize now that the new card component was likely created to address the issue of long titles exceeding the character limit. This is something I hadn't considered initially.

I'd be happy to adjust the existing card component by adding a conditional statement. This would check the word count and automatically adjust the text size to be larger for shorter content and smaller for longer content.

To keep things moving smoothly, I can either open a separate issue for this or jump right in and make the fix here in the PR. Just let me know which approach you prefer!

But if we keep the title text size variable will it not make the UI inconsistent, if in the same page we have heading with variable length

@Michael-Obele
Copy link
Contributor

Hi everyone, I realize now that the new card component was likely created to address the issue of long titles exceeding the character limit. This is something I hadn't considered initially.
I'd be happy to adjust the existing card component by adding a conditional statement. This would check the word count and automatically adjust the text size to be larger for shorter content and smaller for longer content.
To keep things moving smoothly, I can either open a separate issue for this or jump right in and make the fix here in the PR. Just let me know which approach you prefer!

But if we keep the title text size variable will it not make the UI inconsistent, if in the same page we have heading with variable length

You raise a good point about UI consistency. While increasing the base font size slightly could help with longer titles, perhaps a maximum character limit or using ellipsis for overflow could be additional solutions to explore.

@Michael-Obele
Copy link
Contributor

Although I don't see any issue with having a separate card component as we have now, this is only a suggestion.

@TamannaVerma99
Copy link
Contributor Author

TamannaVerma99 commented Mar 20, 2024

Although I don't see any issue with having a separate card component as we have now, this is only a suggestion.

Hey! Actually I have also been working on welcome page ,for there too I have same issues if my title is even "Getting Started" still its covering two lines so it would be really great if we find any possible solution for it .I think we can also pass title size by props.

@benjagm
Copy link
Collaborator

benjagm commented Mar 20, 2024

This code has been already merged in the dev branch. Awesome work!

@benjagm benjagm closed this Mar 20, 2024

const CardBody = ({ title, body, icon, link, image }: CardProps) => {
return (
<div className='group relative h-full w-full max-w-lg rounded-lg border border-gray-200 bg-white p-6 px-12 shadow-3xl transition-colors delay-[150ms] ease-in-out hover:bg-slate-100'>
Copy link
Contributor

Choose a reason for hiding this comment

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

@TamannaVerma99 Please add dark:text-black in the div. As in dark mode the text is not visible.
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants