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

Style changes in homepage #1360

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

cbum-dev
Copy link
Contributor

@cbum-dev cbum-dev commented Jan 22, 2025

What kind of change does this PR introduce?
Ui fixes in main page particularly the "Welcome to the JSON Schema Community" section. Changed the styling and positioning of cards.
Changed more <img tags to Image.

Issue Number:

Screenshots/videos:

Screencast.From.2025-01-22.23-07-02.mp4

If relevant, did you update the documentation?
No

Does this PR introduce a breaking change?

@cbum-dev cbum-dev requested a review from a team as a code owner January 22, 2025 17:39
@cbum-dev
Copy link
Contributor Author

Hi everyone, let me know if their is some need of improvement here.

Copy link

github-actions bot commented Jan 22, 2025

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name Status Preview Last Commit
website ✅ Ready (View Log) Visit Preview 1cbeaed

Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (60f2d6a) to head (1cbeaed).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##              main     #1360   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           10        10           
  Lines          396       396           
  Branches       106       106           
=========================================
  Hits           396       396           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

Can you please review and fix the new borders added? They don't work in the dark theme.
Screenshot 2025-01-25 at 10 35 03

@cbum-dev cbum-dev requested a review from benjagm January 25, 2025 17:10
Copy link
Member

@DhairyaMajmudar DhairyaMajmudar left a comment

Choose a reason for hiding this comment

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

@cbum-dev pls. resolve the merge conflicts

@cbum-dev
Copy link
Contributor Author

@cbum-dev pls. resolve the merge conflicts

Sure.

@benjagm
Copy link
Collaborator

benjagm commented Feb 1, 2025

Hi @cbum-dev Can you please leave the original border style?

Current:
Screenshot 2025-02-01 at 16 55 20

Orginal:
Screenshot 2025-02-01 at 16 55 37

@cbum-dev
Copy link
Contributor Author

cbum-dev commented Feb 1, 2025

Hi @cbum-dev Can you please leave the original border style?

Current: Screenshot 2025-02-01 at 16 55 20

Orginal: Screenshot 2025-02-01 at 16 55 37

Do I have to remove the borders from light mode too ? @benjagm
Also in the dark mode as all you can see the height is not even in all cards and the border is extending. So maybe I have to shorten the height to make the border fit in or this is looking fine?
image

@cbum-dev
Copy link
Contributor Author

cbum-dev commented Feb 4, 2025

Hi @benjagm any update on this?

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.

Looks mostly good. Just left some comments regarding using Image instead img.

/>
</>
)}
<img
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use image instead of img?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @benjagm you mentioned only 2 places where I have to change the img tag or the whole page, as their are so many img tags present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix all of them.

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.

Can you please replace img with Image in all cases? We prefer to use the next Image vs img from HTML.

/>
</>
)}
<img
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix all of them.

@cbum-dev
Copy link
Contributor Author

cbum-dev commented Mar 6, 2025

Can you please replace img with Image in all cases? We prefer to use the next Image vs img from HTML.

Done @DhairyaMajmudar and @benjagm

@DhairyaMajmudar
Copy link
Member

@cbum-dev it seems you need to add some padding in responsive mode, pls. check below screenshot

image

@cbum-dev
Copy link
Contributor Author

@cbum-dev it seems you need to add some padding in responsive mode, pls. check below screenshot

image

image
image
Hi @DhairyaMajmudar is it good enough or I should increase ?

@benjagm
Copy link
Collaborator

benjagm commented Mar 16, 2025

I have a problem with this PR. I don't think we are adding any relevant value, just another style that is not consistent at all with the other cards in the upper part of the section "Why JSON Schema?"

I can accept the proposal of adding the improvements on the styles of the card, but if they are consistent across the landing page.

I don't think the proposed single border improves the style of the cards on top in the section "Why JSON Schema?".

Why not adding a hover style to "Why JSON Schema?" card and apply the same approach in the community cards?

@cbum-dev
Copy link
Contributor Author

I have a problem with this PR. I don't think we are adding any relevant value, just another style that is not consistent at all with the other cards in the upper part of the section "Why JSON Schema?"

Now I think it's true as well. Making them consistent is a bonus.

I don't think the proposed single border improves the style of the cards on top in the section "Why JSON Schema?".
Why not adding a hover style to "Why JSON Schema?" card and apply the same approach in the community cards?

I can think of another idea to make them consistent too, along with adding the hover effect you mentioned.

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.

🐛 Bug: Adding Image tag to all the remaining <img> tags. 🐛 Bug: Ui fixes in main page.
3 participants