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

changed the overflow property of the app.style.js file to hidden #5729

Merged
merged 1 commit into from
Aug 22, 2024

Conversation

BHAV0207
Copy link
Contributor

@BHAV0207 BHAV0207 commented Jul 26, 2024

Description

This PR fixes #5721

Notes for Reviewers
In this PR, I explicitly added styling to the html tag which fixes the responsiveness issue for Devices of different screen sizes. The website's has become more responsive and more aligned to the dimensions of the devices.

  • Yes, I signed my commits.

@l5io
Copy link
Contributor

l5io commented Jul 26, 2024

🚀 Preview for commit ceca5ed at: https://66a3b7e224609a0075d3fa00--layer5.netlify.app

@l5io
Copy link
Contributor

l5io commented Jul 26, 2024

🚀 Preview for commit 16c4301 at: https://66a3c8489bd4590eed255d15--layer5.netlify.app

@Ashparshp
Copy link
Contributor

@BHAV0207 Thanks for your contribution, let's discuss this on the website's call. Please add this as an agenda item to the meeting minutes.

@@ -43,6 +43,7 @@ const GlobalStyle = createGlobalStyle`
}

html{
overflow-x: hidden;
Copy link
Member

Choose a reason for hiding this comment

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

have you tested this all pages and its not having negative effect right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I have tested all pages , on my side there are no negative effects

Copy link
Contributor

@Ashparshp Ashparshp left a comment

Choose a reason for hiding this comment

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

LGTM!! Thanks for making the necessary changes, as this solves the overflow issue for all pages. Great work, @BHAV0207!!

@amitamrutiya
Copy link
Contributor

Hey @sudhanshutech I found one side effect :
https://66a3c8489bd4590eed255d15--layer5.netlify.app/cloud-native-management/meshery/operating-cloud-native-infra

On this page, the right part of the person's image is hidden due to these changes.

image

Wait for @Ashparshp confirmation before making any changes.

@Ashparshp
Copy link
Contributor

@amitamrutiya Thanks, but can you please confirm the width for which you are facing this issue?

@Ashparshp
Copy link
Contributor

@amitamrutiya Thanks, but can you please confirm the width for which you are facing this issue?

Also, I will ask you to recheck as everything seems good on my side.

@amitamrutiya
Copy link
Contributor

Any standard mobile screen size around 450px.

@amitamrutiya
Copy link
Contributor

Weird, I am facing this issue in Firefox but not in Chrome.🤔

@Ashparshp
Copy link
Contributor

Thank you @amitamrutiya. But everything looks good on my side. Please recheck from your side and confirm.

@Ashparshp
Copy link
Contributor

Could you please go through the page at layer5.io//cloud-native-management/meshery/operating-cloud-native-infra and check if the issue still exists? It might not be related to this PR.

@amitamrutiya
Copy link
Contributor

amitamrutiya commented Aug 2, 2024

I am checking on this link https://66a3c8489bd4590eed255d15--layer5.netlify.app/cloud-native-management/meshery/operating-cloud-native-infra (this pr preview url) at firefox.

image

In chrome everything looks perfect.
image

@amitamrutiya
Copy link
Contributor

By the way not a big issue. I think there is something not going well on my side. No worries :) ☺️

@Ashparshp
Copy link
Contributor

The issue you are facing is not related to this PR; (it existed before this PR and persists in Firefox). Thank you so much for your review. Keep it up!

@Ashparshp
Copy link
Contributor

@BHAV0207 Thanks for your contribution, let's discuss this on the website's call. Please add this as an agenda item to the meeting minutes.

@Muhammed-Moinuddin
Copy link
Contributor

Specifically for the Homepage, what I found out is that just by removing one of the section i.e. (Deploy Faster Together With Confidence). The horizontal scrolling issue is resolved. So I think we just need to solve that section instead of adding it directly for the whole website.
@Ashparshp @sudhanshutech

@Ashparshp
Copy link
Contributor

@Muhammed-Moinuddin, we're doing this because many pages in the handbook section have horizontal scrolling issues. By addressing this, we can fix all of them at once instead of solving for each page separately. I hope that's clear!

@BHAV0207
Copy link
Contributor Author

@sudhanshutech can you please review the changes

@Ashparshp
Copy link
Contributor

@Muhammed-Moinuddin, @hargunkaur286, @amitamrutiya, @BHAV0207, do you have anything to say, or are we good to go with this PR?

@BHAV0207
Copy link
Contributor Author

@Muhammed-Moinuddin, @hargunkaur286, @amitamrutiya, @BHAV0207, do you have anything to say, or are we good to go with this PR?

from my side it's good to go.

@Muhammed-Moinuddin
Copy link
Contributor

@Muhammed-Moinuddin, @hargunkaur286, @amitamrutiya, @BHAV0207, do you have anything to say, or are we good to go with this PR?

This seems fine to me. We can move forward with it.

But... I got a small question that if we look another side of the picture, how we are going to find out responsiveness issues created due to any components overflow? This will make us to ignore those issues even they existed due to not making them properly responsive etc...
Thoughts?

@amitamrutiya
Copy link
Contributor

LGTM as well.

But... I got a small question that if we look another side of the picture, how we are going to find out responsiveness issues created due to any components overflow? This will make us to ignore those issues even they existed due to not making them properly responsive etc...
Thoughts?

Unfortunately, we’re stuck with just manual testing for now. 😔🔧

@Ashparshp
Copy link
Contributor

@Muhammed-Moinuddin, @hargunkaur286, @amitamrutiya, @BHAV0207, do you have anything to say, or are we good to go with this PR?

This seems fine to me. We can move forward with it.

But... I got a small question that if we look another side of the picture, how we are going to find out responsiveness issues created due to any components overflow? This will make us to ignore those issues even they existed due to not making them properly responsive etc... Thoughts?

The horizontal overflow will be hidden for now, but we should identify the root cause of the overflow issue. If anyone has a better solution or is willing to investigate this further, that would be great. If you think this approach might cause problems, feel free to suggest an alternative. Is anyone up for investigating or coming up with a better approach?

@Ashparshp
Copy link
Contributor

@amitamrutiya, do we need automation testing for this? If yes and you're up for this then let me know.

@amitamrutiya
Copy link
Contributor

I am not familiar with automation testing for this.

@Ashparshp
Copy link
Contributor

@amitamrutiya, alright! No issues.

@vishalvivekm vishalvivekm merged commit 8d9b63f into layer5io:master Aug 22, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Website not correctly aligned according to different devices.
7 participants