-
Notifications
You must be signed in to change notification settings - Fork 90
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
Refactor feature section with parallax effectFeature section refactor #4
Conversation
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
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.
good initiative @sahadat-sk !
- There's a lot of empty space over the first feature.
- Once you scroll to the second feature the GIF transition should happen once the text of second feature has come to the middle of the first image.
- In the small screen size this is completely breaking, please check the responsiveness.
…-refactor Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
deb7d38
to
372f779
Compare
Hey @nehagup can you review now? |
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.
@sahadat-sk Please revert back the linting changes
Hey @sahadat-sk Smooth transition kudos!
|
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
Hey @Hermione2408, I have made the requested changes, can you review them now? |
components/features.tsx
Outdated
</div> | ||
</div> | ||
</div> | ||
</section> | ||
<div className="max-w-6xl mx-auto mb-24 -mt-24 text-center"> |
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 @sahadat-sk everything looks fine just remove this division it is not going well with the UI
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.
Done
Signed-off-by: sahadat-sk <[email protected]>
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 @nehagup
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.
Thanks for the PR @sahadat-sk ! A few changes.
- The section is breaking on md and sm screen sizes, please stick to the same old format for md and sm screen size.
- User needs to much scrolling from 1 feature to other, the second feature should start to appear as the first feature text comes to the top of container (attached screenshot)
- can you please center align the 3rd GIF in center?
- there's a lot of empty space after 3rd feature ends. Please reduce that to margins same as margins in other sections.
Rest looks good!
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
Signed-off-by: sahadat-sk <[email protected]>
…-refactor Signed-off-by: sahadat-sk <[email protected]>
333cacb
to
c6e5544
Compare
I have made the changes, can you please review them!! |
…-refactor Signed-off-by: sahadat-sk <[email protected]>
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.
@sahadat-sk Everything else looks good! Just that First feature fades away before the first GIF comes even to the center. It's fine to remove the 3rd GIF and scroll the features-text with just 2 GIFs so that text gets more time and GIFs doesn't change too soon.
Signed-off-by: sahadat-sk <[email protected]>
Hey @nehagup, I have made the changes, do let me know if there is anything else I need to change! |
Fixes: #1674
Demo (Sorry for laggy video 😅):
keploy.webm