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: expand container width on large screen #62

Merged
merged 3 commits into from
Feb 20, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Feb 2, 2021

I think we can borrow this technique from Bootstrap.

Jest blog (need to resize columns as 3/7/2, to close facebook/docusaurus#4083):

Before After
image image

Docs:

Before After
image image

@lex111 lex111 requested a review from slorber February 2, 2021 13:35
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Feb 2, 2021
@slorber
Copy link
Collaborator

slorber commented Feb 2, 2021

Thanks, That looks better to me.

Was wondering if it's not worth keeping the content perfectly centered? (not even sure it is right now 😅 )

@lex111
Copy link
Contributor Author

lex111 commented Feb 2, 2021

Actually not, but maybe I don't properly understand what you mean.

@slorber
Copy link
Collaborator

slorber commented Feb 2, 2021

For example:

image

I mean we may want the main content to have the same margin on the right and left sides, is using 2/8/2 or 3/6/3 and not 1/8/3

I'm not a designer so not sure what is a good choice here 😅

Increasing the container width for larger screens looks safe to me, but can't really tell for moving to 3/7/2 as it's a design decision

@lex111
Copy link
Contributor Author

lex111 commented Feb 2, 2021

Well, it might be worth discussing this with other team members later.

Changing columns to 3/7/2 applies only to blog pages, this is necessary to increase the block size with recent posts.

@lex111 lex111 mentioned this pull request Feb 8, 2021
@yangshun
Copy link
Contributor

yangshun commented Feb 8, 2021

I'll review this

@lex111 lex111 requested a review from yangshun February 14, 2021 16:12
@yangshun yangshun force-pushed the lex111/expand-container-large-screen branch from 9a3280c to 27a066a Compare February 19, 2021 15:17
@netlify
Copy link

netlify bot commented Feb 19, 2021

Deploy preview for infima ready!

Built with commit 5d41d4a

https://deploy-preview-62--infima.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 19, 2021

Size Change: +996 B (0%)

Total Size: 528 kB

Filename Size Change
./packages/core/dist/css/default-dark/default-dark-rtl.css 76.6 kB +139 B (0%)
./packages/core/dist/css/default-dark/default-dark-rtl.min.css 53.3 kB +110 B (0%)
./packages/core/dist/css/default-dark/default-dark.css 76.6 kB +139 B (0%)
./packages/core/dist/css/default-dark/default-dark.min.css 53.3 kB +110 B (0%)
./packages/core/dist/css/default/default-rtl.css 75.8 kB +139 B (0%)
./packages/core/dist/css/default/default-rtl.min.css 52.8 kB +110 B (0%)
./packages/core/dist/css/default/default.css 75.7 kB +139 B (0%)
./packages/core/dist/css/default/default.min.css 52.8 kB +110 B (0%)
ℹ️ View Unchanged
Filename Size Change
./packages/core/dist/js/alerts.js 409 B 0 B
./packages/core/dist/js/alerts.min.js 409 B 0 B
./packages/core/dist/js/button-groups.js 267 B 0 B
./packages/core/dist/js/button-groups.min.js 267 B 0 B
./packages/core/dist/js/dropdowns.js 1.01 kB 0 B
./packages/core/dist/js/dropdowns.min.js 1.01 kB 0 B
./packages/core/dist/js/menu.js 1.74 kB 0 B
./packages/core/dist/js/menu.min.js 1.74 kB 0 B
./packages/core/dist/js/navbar.js 926 B 0 B
./packages/core/dist/js/navbar.min.js 926 B 0 B
./packages/core/dist/js/pills.js 270 B 0 B
./packages/core/dist/js/pills.min.js 270 B 0 B
./packages/core/dist/js/radio-behavior.js 705 B 0 B
./packages/core/dist/js/radio-behavior.min.js 705 B 0 B
./packages/core/dist/js/tabs.js 267 B 0 B
./packages/core/dist/js/tabs.min.js 267 B 0 B

compressed-size-action


@media (min-width: 1440px) {
& {
--ifm-container-width: 1320px;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should give people a way to modify this just by tweaking CSS variables. If we do this then they have to write a new media query. I think we should define a CSS variable for this viewport so people can override it if they want to:

--ifm-container-width: 1140px;
--ifm-container-width-xl: 1320px;

...

 @media (min-width: 1440px) {
    & {
      max-width: var(--ifm-container-width-xl);
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, fixed.

@lex111 lex111 force-pushed the lex111/expand-container-large-screen branch from 27a066a to 5d41d4a Compare February 19, 2021 17:09
@slorber
Copy link
Collaborator

slorber commented Feb 19, 2021

For some weird reason I see this change applied to the demo page, but not the infima website deploy preview 😅 any idea why?

Also, I think it's an improvement, but don't like too much having containers "jump size" after a certain threshold. What if the container would just grow naturally between 1200px and 1440px, using a % value?

@lex111
Copy link
Contributor Author

lex111 commented Feb 19, 2021

Also, I think it's an improvement, but don't like too much having containers "jump size" after a certain threshold. What if the container would just grow naturally between 1200px and 1440px, using a % value?

What do you have in mind exactly? Can you show the code for implementing this? Normally, the screen has a fixed size, so I don't understand how container can jump in this case, unless you manually change screen size, for example, for development purposes.

Copy link
Contributor

@yangshun yangshun left a comment

Choose a reason for hiding this comment

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

Nice one, thank you!

@yangshun
Copy link
Contributor

For some weird reason I see this change applied to the demo page, but not the infima website deploy preview 😅 any idea why?

Is the website deploy preview using the trunk version of Infima? Otherwise it won't show up there.

@yangshun yangshun merged commit f1e03ce into master Feb 20, 2021
@yangshun yangshun deleted the lex111/expand-container-large-screen branch February 20, 2021 04:27
@slorber
Copy link
Collaborator

slorber commented Feb 22, 2021

Is the website deploy preview using the trunk version of Infima? Otherwise it won't show up there.

Everything is using symlinks and I can see the changes locally so not sure what is happening with the deployed preview 😅
Will see on next changes if it works or not but it is supposed to deploy the infima changes on the website

@slorber
Copy link
Collaborator

slorber commented Feb 22, 2021

What do you have in mind exactly? Can you show the code for implementing this? Normally, the screen has a fixed size, so I don't understand how container can jump in this case, unless you manually change screen size, for example, for development purposes.

I mean, if the width is 1439px, do we really want the container to be significantly smaller than for width=1440px?

I do agree that resizing is not something done by regular users too much and the experience does not matter much, just saying that there's a way to optimize the width we render to for "middle width" devices by using a % value. Like if width > 1300px && width < 1440px ) container-width=90%.

Not too important for now

@slorber
Copy link
Collaborator

slorber commented Mar 4, 2021

@lex111 do we want an even larger container for larger screens?

Jest reported truncated TOC:
https://jestjs.netlify.app/docs/mock-function-api

Even using the new xl container width it gets truncated for very large screens and feels like we are wasting some space.

image

@lex111
Copy link
Contributor Author

lex111 commented Mar 4, 2021

@slorber it's hard to say, maybe in the docs layout we should adopt columns similar structure as in the blog? Or set even larger width for docs container, but I don't figure out what it should be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve blog layout
4 participants