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

styles: updates the home page #93

Merged
merged 15 commits into from
Oct 9, 2024
Merged

Conversation

pavanydg
Copy link
Contributor

@pavanydg pavanydg commented Oct 6, 2024

What kind of change does this PR introduce?
enhances the home page.

Issue Number:

Screenshots/videos:

Screencast.from.10-06-2024.10.11.02.PM.webm

If relevant, did you update the documentation?

Summary

Does this PR introduce a breaking change?

@pavanydg pavanydg closed this Oct 6, 2024
@pavanydg pavanydg reopened this Oct 6, 2024
@pavanydg
Copy link
Contributor Author

pavanydg commented Oct 6, 2024

@JeelRajodiya pl review.

Copy link
Member

@gregsdennis gregsdennis left a comment

Choose a reason for hiding this comment

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

As per my comments in the issue, the design of this page needs to match that of the home page: https://json-schema.org/. This design does not match that. Just glancing at it, I can tell the font doesn't even match.

We need to agree on a design before work is done.

@pavanydg
Copy link
Contributor Author

pavanydg commented Oct 6, 2024

ok will update once the new design is finalized. Sorry for pulling a request without asking.

@gregsdennis
Copy link
Member

@pavanydg you had asked, and that's great. But there was conversation on the issue that had not been resolved. I don't want to you feel you broke some rule; I just don't want you to feel like you wasted your time. We discuss the change that needs to happen, then we implement the change.

Copy link
Member

@JeelRajodiya JeelRajodiya Oct 7, 2024

Choose a reason for hiding this comment

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

It's Great, can you please use CSS grid of 4 rows each, this can make the links dynamic, incase we need to add more additional chapters we won't need to modify this component.

This video can help: https://www.youtube.com/watch?v=705XCEruZFs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks I will do it once the design is finalized.

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 i tried using grid, but the thing is it fills row wise. So the outcome will look like
1.Getting Started 2.PrimitiveTypes
and then from next line. So i have modified the previous code such that if we add additional chapters it works fine.

Copy link
Member

@JeelRajodiya JeelRajodiya Oct 8, 2024

Choose a reason for hiding this comment

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

You can resolve the row wise filling issue using the grid-auto-flow CSS property. Let me know if that does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that property is not working as desired

Copy link
Member

@JeelRajodiya JeelRajodiya Oct 8, 2024

Choose a reason for hiding this comment

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

it's okay. You can continue working on the rest of the design.

@pavanydg
Copy link
Contributor Author

pavanydg commented Oct 8, 2024

@JeelRajodiya pl review.

Copy link
Member

@JeelRajodiya JeelRajodiya left a comment

Choose a reason for hiding this comment

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

It looks great on 80% zoom

image

but when I zoom to 100% some of the links become difficult to read, please see the below screenshot

image

Can you please resolve it? (please consider testing with other zoom levels e.g. 110%, 120% etc. )

@pavanydg
Copy link
Contributor Author

pavanydg commented Oct 9, 2024

@JeelRajodiya pl review.

@JeelRajodiya
Copy link
Member

JeelRajodiya commented Oct 9, 2024

It's perfect! Thank you for your contribution @pavanydg
I will merge the PR in a while.

@JeelRajodiya JeelRajodiya changed the title Updates the home page styles: updates the home page Oct 9, 2024
@JeelRajodiya JeelRajodiya merged commit 888447a into json-schema-org:main Oct 9, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update the home page
3 participants