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.

Sign up for GitHub

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

Jump to bottom

Latest fix #428: Added hover effect for buttons #497

Merged
merged 10 commits into from
Apr 8, 2024
Merged

Latest fix #428: Added hover effect for buttons #497

merged 10 commits into from
Apr 8, 2024

Conversation

VivekJaiswal18
Copy link
Contributor

What kind of change does this PR introduce?
feature fix

Issue Number:
Closes #428
Latest fix considering the Feedback provided in the review
Does this PR introduce a breaking change?
No

@VivekJaiswal18 VivekJaiswal18 mentioned this pull request Mar 10, 2024
Closed
DhairyaMajmudar
DhairyaMajmudar reviewed Mar 10, 2024
View reviewed changes
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.

Pls. remove the changes from yarn.lock and package.lock.json file so that lint checks get passed.

@DhairyaMajmudar
Copy link
Member

Vivek pls. don't delete yarn.lock file , discussion here will help you #498

@VivekJaiswal18
Copy link
Contributor Author

VivekJaiswal18 commented Mar 10, 2024 •
edited
Loading

@DhairyaMajmudar So i should be pushing the yarn.lock file to the same branch so that it gets added again right?

@DhairyaMajmudar
Copy link
Member

Yes correct you have to push back the yarn.lock file also, instead of using npm commands used yarn commands so that package.lock.json and yarn.json will not modify : )

@VivekJaiswal18
Copy link
Contributor Author

Yes correct you have to push back the yarn.lock file also, instead of using npm commands used yarn commands so that package.lock.json and yarn.json will not modify : )

Got it.
But there is no package.lock.json file when you use yarn it only gets added when you are using npm

@DhairyaMajmudar
Copy link
Member

True that's why yarn commands are supposed to be used to avoid package.lock.json

@VivekJaiswal18
Copy link
Contributor Author

True that's why yarn commands are supposed to be used to avoid package.lock.json

Yes you're correct!

@benjagm
Copy link
Collaborator

benjagm commented Mar 11, 2024

@VivekJaiswal18 can you check the errors of the CI/CD pipeline please?

https://github.com/json-schema-org/website/actions/runs/8224840411/job/22489137644?pr=497

benjagm
benjagm requested changes Mar 11, 2024
View reviewed changes
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.

Thanks for the hard work. You'll need to check the errors of the CI/CD pipeline:

https://github.com/json-schema-org/website/actions/runs/8224840411/job/22489137644?pr=497

@VivekJaiswal18
Copy link
Contributor Author

Okay checking that

@lalitkumawat1m
Copy link
Contributor

@VivekJaiswal18 are you facing any issue in fixing linting error ?

@VivekJaiswal18 VivekJaiswal18 requested a review from a team as a code owner March 17, 2024 17:55
DhairyaMajmudar
DhairyaMajmudar reviewed Mar 20, 2024
View reviewed changes
pages/index.page.tsx Outdated
>
Join Slack
</Link>

<div className='flex herobtn items-center justify-center font-semibold w-[194px] h-[40px] rounded border-2 border-white text-white mx-auto dark:shadow-2xl'>

Copy link
Member

Choose a reason for hiding this comment

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

Hi @VivekJaiswal18, your PR is almost complete just one lint check is failing can you pls. fix it.

@VivekJaiswal18
Copy link
Contributor Author

@benjagm I have rebased the branch please have a look.
Thank you

DhairyaMajmudar
DhairyaMajmudar requested changes Mar 26, 2024
View reviewed changes
pages/index.page.tsx Outdated
/>
<p>Search</p>
</div>
<div className='herobtn rounded border-2 border-white hover:bg-blue-700 transition-all duration-300 ease-in-out text-white mx-auto'>
Copy link
Member

Choose a reason for hiding this comment

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

@VivekJaiswal18 pls. have a look on this change. because after applying this the search bar UI is not as expected.

image

pages/index.page.tsx Outdated
@@ -335,7 +332,7 @@ const Home = (props: any) => {
<h2 className='text-h3mobile lg:text-h3 text-white mb-6'>
Start learning JSON Schema
</h2>
<button className='w-[170px] h-[45px] mx-auto rounded border-2 bg-primary text-white font-semibold dark:bg-[560bad] dark:border-none'>
<button className='w-[170px] h-[45px] mx-auto hover:bg-blue-700 transition-all duration-300 ease-in-out rounded border-2 bg-primary text-white font-semibold'>
Copy link
Member

Choose a reason for hiding this comment

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

Here you can apply dark:bg-transparent and dark:hover:bg-blue-700 class to make the button similar to the above buttons in dark theme.

Copy link
Contributor Author

@VivekJaiswal18 VivekJaiswal18 Mar 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I get what you are trying to suggest but I think having it colored makes it visually easy to identify and in the deployed version also it is colored and not following a skeleton design. So changing that would be beyond the scope for this particular enhancement: adding hover effect and will create confusion in future.

pages/index.page.tsx Outdated
@@ -212,26 +212,23 @@ const Home = (props: any) => {
<div className='lg:w-[650px] mx-auto my-10 grid grid-cols-1 lg:grid-cols-3 gap-8 justify-items-center '>
<Link
href='/learn/getting-started-step-by-step'
className=' flex items-center justify-center rounded border-2 border-white dark:border-none text-white w-[194px] h-[40px] font-semibold bg-primary dark:shadow-2xl'
className='flex items-center justify-center rounded border-2 border-white hover:bg-blue-700 transition-all duration-300 ease-in-out text-white w-[194px] h-[40px] font-semibold'
Copy link
Member

Choose a reason for hiding this comment

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

Here instead of removing bg-primary you can add dark:bg-transparent since the buttons are a bit darkish blue in light mode which is removed after removing bg-primary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here as in dark mode dark:border-none there is no border to differentiate the button from the background adding dark:bg-transparent will overwrite and remove the bg color as well making it look like
image
So only adding bg-primary is fine

benjagm
benjagm approved these changes Apr 8, 2024
View reviewed changes
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.

Good job! Just pushed a small change to make sure the change available in production to center the content of the search button of the hero page is available here too.

@benjagm benjagm merged commit 210007f into json-schema-org:main Apr 8, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DhairyaMajmudar DhairyaMajmudar DhairyaMajmudar requested changes

@benjagm benjagm benjagm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

✨ Enhancement: Adding hover effect on the buttons.
4 participants
@VivekJaiswal18 @DhairyaMajmudar @benjagm @lalitkumawat1m