refactor(v2): improve semantic markup of blog #2003
Conversation
lex111
added
the
pr: polish
|
Deploy preview for docusaurus-2 ready! Built with commit 83eeafc |
|
Deploy preview for docusaurus-preview ready! Built with commit 83eeafc |
| @@ -21,16 +21,14 @@ function BlogListPage(props) { | |||
| <div className="col col--8 col--offset-2"> | |||
| {items.map( | |||
| ({content: BlogPostContent, metadata: blogPostMetadata}) => ( | |||
| <div | |||
There was a problem hiding this comment.
What's wrong with a wrapper div?
There was a problem hiding this comment.
It is not needed, we can insert the CSS class directly into the article tag, why an extra tag (div)?
There was a problem hiding this comment.
I mean, the <div> is there so that we can add margin. If we don't need to add margin of course not having a div is better.
| @@ -92,13 +93,13 @@ function BlogPostItem(props) { | |||
| }; | |||
|
|
|||
| return ( | |||
| <div> | |||
| <article {...attrs}> | |||
There was a problem hiding this comment.
No, we don't want to channel props through components, it makes it harder to know what props are being used and which aren't. Accepting className props is also an anti-pattern that we don't want to encourage. If article defines its own className in future, it will work when the parents don't pass in a className but will break when the parents do. This kind of bugs are hard to detect and reliably prevent, so we don't want to allow that at all.
There was a problem hiding this comment.
Accepting className props is also an anti-pattern that we don't want to encourage.
And what should I do then?
There was a problem hiding this comment.
If article defines its own className in future, it will work when the parents don't pass in a className but will break when the parents do.
We can merge classes with classnames lib, right? Why is that bad? I agree with passing rest props, but why can't extract className if all CSS classes are merged later?
There was a problem hiding this comment.
Used a different approach, I hope it will be good for you 83eeafc
There was a problem hiding this comment.
We can merge classes with classnames lib, right? Why is that bad?
Which class get priority? The component's classes or the user's classes? Either approach is bad as it leads to unpredictable behavior. In this scenario:
.user-class { padding: 0; }
.component-class { padding: 12px; }
Both classes affect padding and again this makes the final result dependent on CSS ordering. Imagine the user added background-color: red to .user-class and in a future release we add background-color: orange to .component-class, it could break the user's CSS. Also, since we use webpack with code splitting, the CSS loading order is hard to reason about and can lead to obscure bugs.
The only way is to not allow any merging of CSS classes. This is our practice for Facebook components. For common properties that people like to override, we allow passing in of margin/padding props. What you've done here is acceptable as we're still in control.
There was a problem hiding this comment.
Thanks for the clarification, in this case, the situation is somewhat different, since we are dealing with an internal component, I mean, an end user can set own CSS class only if component will be swizzled, initially I did not take this fact into account.
| @@ -21,16 +21,14 @@ function BlogListPage(props) { | |||
| <div className="col col--8 col--offset-2"> | |||
| {items.map( | |||
| ({content: BlogPostContent, metadata: blogPostMetadata}) => ( | |||
| <div | |||
There was a problem hiding this comment.
I mean, the <div> is there so that we can add margin. If we don't need to add margin of course not having a div is better.
Motivation
Fully semantic layout of blog pages. Search robots will be satisfied 😃
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
See HTML markup, no UI changes.