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

refactor: make dynamic authors layout via CSS only #5424

Merged
merged 2 commits into from
Aug 26, 2021

Conversation

lex111
Copy link
Contributor

@lex111 lex111 commented Aug 26, 2021

Motivation

When we have two-column layout for authors, we can remove JS logic of adding right class depending of authors numbers -- just we need to add little extra CSS code to get the same result.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Preview.

2021-08-26_14-14

2021-08-26_14-14_1

Related PRs

(If this PR adds or changes functionality, please take some time to update the docs at https://github.com/facebook/docusaurus, and link to your PR here.)

@lex111 lex111 added the pr: bug fix This PR fixes a bug in a past release. label Aug 26, 2021
@lex111 lex111 requested a review from slorber as a code owner August 26, 2021 11:35
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Aug 26, 2021
@netlify
Copy link

netlify bot commented Aug 26, 2021

✔️ [V2]

🔨 Explore the source changes: becb869

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61277c65510444000813bb4d

😎 Browse the preview: https://deploy-preview-5424--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Aug 26, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟢 Performance 98
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5424--docusaurus-2.netlify.app/

@github-actions
Copy link

github-actions bot commented Aug 26, 2021

Size Change: +189 B (0%)

Total Size: 814 kB

Filename Size Change
website/build/assets/css/styles.********.css 95 kB +66 B (0%)
website/build/blog/index.html 37 kB +116 B (0%)
ℹ️ View Unchanged
Filename Size Change
website/.docusaurus/globalData.json 36.2 kB 0 B
website/build/assets/js/main.********.js 410 kB 0 B
website/build/blog/2017/12/14/introducing-docusaurus/index.html 66 kB +14 B (0%)
website/build/docs/index.html 43.1 kB -3 B (0%)
website/build/docs/installation/index.html 50.7 kB -4 B (0%)
website/build/index.html 29.8 kB 0 B
website/build/tests/docs/index.html 24.1 kB 0 B
website/build/tests/docs/standalone/index.html 21.9 kB 0 B

compressed-size-action

@netlify
Copy link

netlify bot commented Aug 26, 2021

✔️ [V2]

🔨 Explore the source changes: bd4744b

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61277cba0f66ff0008c4377a

😎 Browse the preview: https://deploy-preview-5424--docusaurus-2.netlify.app

Copy link
Collaborator

@slorber slorber left a comment

Choose a reason for hiding this comment

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

I must admit I'm not too fan of using important!.

If we don't strictly adhere to Infima's grid, why use it in the first place instead of just flex: 0 1 50% ?

Also my idea was to allow swizzlers to easily move to a 3-col layout if they want to (but agree this may seem more complicated and a bit overkill).

No strong opinions, I'm ok to merge this if you want but I'm not sure yet it's the right thing to do.

@lex111
Copy link
Contributor Author

lex111 commented Aug 26, 2021

I like to change existing column, although this will require using !important. It seems sort of in a more proper, using same grid system, but just add the ability for column to expand to full width.

On the other hand, we can define your own column, but we still need to use !important, and wrap the CSS rule in media query, because columns should be displayed in full width on mobiles.

@media only screen and (min-width: 997px) {
  .authorCol {
    flex: 1 1 50% !important;
  }
}

So which option is better?

As for swizzling, it's a pretty rare use case that we don't particularly recommend; so I'd use usual CSS-like solution to make layout, and avoid having to use JS to do it.

Also my idea was to allow swizzlers to easily move to a 3-col layout if they want to (but agree this may seem more complicated and a bit overkill).

Well that's pretty ambiguous, it might be easier for user to set up three-column layout using CSS-grid, rather than JS.

@slorber
Copy link
Collaborator

slorber commented Aug 26, 2021

ok, let's merge this for now

@slorber slorber merged commit 1f1c7f1 into main Aug 26, 2021
@slorber slorber deleted the lex111/ref-blog-post-mul-css branch August 26, 2021 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: bug fix This PR fixes a bug in a past release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants