-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Modifying integration container in row in mobile view #4684
Modifying integration container in row in mobile view #4684
Conversation
Signed-off-by: Cvr421 <[email protected]>
Hii @thisiskaransgit I have made the changes to the issue #4665 . Please review and let me know your thoughts. |
🚀 Preview for commit 6f15c13 at: https://64ca1a012916e27c3b031e26--layer5.netlify.app |
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.
@Cvr421, the honeycomb section should be responsive, the containers are stretched with the horizontal scroll, please fix that.
Hii @thisiskaransgit. Like this way we want ? |
Signed-off-by: Cvr421 <[email protected]>
Hii @thisiskaransgit , I have made the changes requested by you please review and let me know anything need to be modify. |
🚀 Preview for commit 4104a24 at: https://64ccd2888a3f3873aa193445--layer5.netlify.app |
Hey @Cvr421 Let's discuss it on the websites call. |
ok @Ghat0tkach let's discuss it on meeting minutes. |
🚀 Preview for commit a634483 at: https://64d0e42ef1f1826c2de8a8c8--layer5.netlify.app |
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit c252264 at: https://64d23bd5af4bd707fefe2b55--layer5.netlify.app |
Hii @thisiskaransgit i have added some padding and reduce the size of integration container . Please review and let me know anything need to change . These changes will be reflect when the screen width size is less then 340px. |
@thisiskaransgit thanks for all your work on reviewing these PRs. 👏 |
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.
@Cvr421, I am unable to reproduce the update on my mobile devices, since the max-width used is small(width of smaller mobiles). Make the changes available for the width of ~425px, please make sure the margin between the containers remains consistent. Let's make this the final review and conclude this task.
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit 105ce0a at: https://64d785ba0c870d0261252015--layer5.netlify.app |
Hii @thisiskaransgit I have made the changes requested by you . please review |
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.
@Cvr421, font-size...😬
Sorry @thisiskaransgit font size was working fine in my local environment. let me fix it quickly. |
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit 5b0a07e at: https://64d827ecc2cd7d4d18b60db0--layer5.netlify.app |
Hii @thisiskaransgit I have fixed the text overflow . please check |
@Cvr421 Let's discuss it on the websites call. Please add this as an agenda item in the meeting minutes, if you would :) |
ok @vishalvivekm i'll do it. |
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit 7869657 at: https://64da723b202f271394b89c17--layer5.netlify.app |
Hii @thisiskaransgit I have made some improvised changes. Please review |
Hii @Chadha93 @vishalvivekm @thisiskaransgit . please review this PR . I have checked on my mobile device. it's working good. |
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.
Looks good to merge, thank you for your contribution @Cvr421 🎉
|
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.
@Cvr421 will you please investigate the occasional overlap that seems to continue occur?
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit 381665c at: https://64e5d56f52371c0079ad3d71--layer5.netlify.app |
Hii @leecalcote please make sure that the changes i have made will only apply . when the screen size is less then 425px in mobile view. And i have checked but i have not found overlapping with each other. when screen size is less then 425px . But i have made some changes into the container height . So that it will not overlap in any condition . when the screen size is less then 425px. |
Thanks @leecalcote for video explanation . I think this is happening specific with safari browser . Because I have checked on Chrome and Firefox and I have not found overlapping . Sorry I don't have access to safari . But if you want I can increase the Media query max-width to 500px to fix it . @thisiskaransgit has suggested to keep media query max-width to 425px. |
Hii @leecalcote can you please review this PR . |
Signed-off-by: Cvr421 <[email protected]>
🚀 Preview for commit d48e2ee at: https://650b6ab1a26a065b92039960--layer5.netlify.app |
@leecalcote Sorry for delay . Because I have been working on other project |
🚀 Preview for commit 71b84cc at: https://650dbfd1e7943d0a3ae2d2e0--layer5.netlify.app |
Signed-off-by: Chandravijay rai <[email protected]>
🚀 Preview for commit a8cbb79 at: https://653fb2dc2440af1a8e11591b--layer5.netlify.app |
@Cvr421 bring this item to website meeting and ask if we still needed or not |
@sudhanshutech I have discuss this no of time in meeting. actually the issue is after the screen width size of 425px the Honeycomb structure is overlapping with each other on Safari browser . Can you Please check this changes https://653fb2dc2440af1a8e11591b--layer5.netlify.app/ that after 425px it overlapping or not On safari browser if you have safari . Because I don't have Safari that's why I am not able to work properly. other wise below 425px it is working good. |
Text animations aren't smooth on safari |
@Ckbhatia have you check above 425px . and please expand all integration and check if their is overlapping or not . above 425px |
@@ -4,6 +4,8 @@ import { HoneycombGrid } from "./Honeycomb.style"; | |||
const Honeycomb = (props) => { | |||
const { items, renderItem } = props; | |||
const [height, setHeight] = useState(0); | |||
const [heightMobile, setHeightmobile]= useState(0); |
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.
Why are these extra height states required?
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.
Because when you see all the integration then in bottom the honeycomb structure is overlapping with other next component . and also the height is depend on honey comb size that why we need separate height for mobile screen .
Yes, working well on the above screens while expanded as well |
As discussed on meeting that current design looks good, until we have a strong input or design here we can keep it as usual otherwise this thing will get streched. Thanks @Cvr421 |
Description
This PR fixes #4665
Notes for Reviewers
Current Behaviour
Expected Behaviour
Signed commits
Yes, I signed my commits.