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

feat: add different width for different media for main content #473

Merged
merged 6 commits into from
Aug 23, 2024

Conversation

Revathyvenugopal162
Copy link
Contributor

@Revathyvenugopal162 Revathyvenugopal162 commented Aug 22, 2024

fix #325

Now

image

before

image

@github-actions github-actions bot added the bug Defects or glitches reported by users or developers label Aug 22, 2024
@SMoraisAnsys
Copy link
Contributor

The changes are great, I was also thinking that a lot of space was "wasted" on the sides.
Would it be possible to be even greedier than what you did already ? :D

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

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

Looks good! I wouldn't be greedier though like @SMoraisAnsys mentioned 😄 you might start finding issues if you stick too much to the left side... 😄


@media (min-width: 960px) {
.bd-page-width {
max-width: min(100%, 1600px) !important;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SMoraisAnsys, @RobPasMue increase this size to 1700px will increase the screen size, since 1920 is the standard resolution , i can increase this if you prefer (if you are greedy)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The result is great ! Is there a reason for the middle part to be that far away from the scroll bar ?
As far as I like this, I don't have enough hindsight to assess the extent to which this could cause problems (which @RobPasMue pointed out).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result is great ! Is there a reason for the middle part to be that far away from the scroll bar ? As far as I like this, I don't have enough hindsight to assess the extent to which this could cause problems (which @RobPasMue pointed out).

good catch, i reduced the gap
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is going to far.... you probably dont want to have so much space between el central content and the right bar, and so little space between the central content and the scrollbar.....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The left and right content is having the same margin and padding, and the scroll bar appears only if there is an overflow in the content.
image
image
These are the 2 changes with and without scrollbar. pinging @mia-guo-ux also here for insights

Choose a reason for hiding this comment

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

Thanks for pinning me. When the text exceeds the container, I suggest truncating the text with "..." and show the full content upon hovering instead of enabling horizontal scrolling. The vertical scrollbar looks good to me. Also...I agree with the comments that balance is the key :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you all, Then i will go with the above specifications, and about horizontal scrollbar, i will open another PR to fix.

Copy link
Contributor

@germa89 germa89 left a comment

Choose a reason for hiding this comment

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

I like the new change... but:

  • Don't go too far with it, it will clutter the view.
  • Don't stick too close to any of the sides while you have plenty of space on the other side. You want to strick a balance between both sides.

But overall, I like it. Great job!

@Revathyvenugopal162 Revathyvenugopal162 marked this pull request as ready for review August 22, 2024 14:11
@Revathyvenugopal162 Revathyvenugopal162 requested a review from a team as a code owner August 22, 2024 14:11
@RobPasMue
Copy link
Member

  • Don't go too far with it, it will clutter the view.
  • Don't stick too close to any of the sides while you have plenty of space on the other side. You want to strick a balance between both sides.

Balance is the key!

@germa89
Copy link
Contributor

germa89 commented Aug 22, 2024

asdf

@Revathyvenugopal162 Revathyvenugopal162 enabled auto-merge (squash) August 23, 2024 07:31
@Revathyvenugopal162 Revathyvenugopal162 merged commit 73668d8 into main Aug 23, 2024
23 checks passed
@Revathyvenugopal162 Revathyvenugopal162 deleted the fix/media-width branch August 23, 2024 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defects or glitches reported by users or developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doc template not using the entire screen
6 participants