-
Notifications
You must be signed in to change notification settings - Fork 22
fix(BlockHeader): refactoring #105
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
Conversation
|
Preview is ready. |
src/components/HeaderComponent/__stories__/HeaderComponent.stories.tsx
Outdated
Show resolved
Hide resolved
CHANGELOG.md
Outdated
| ## 0.18.4 | ||
|
|
||
| - Fixed the `BlockHeader` margins. | ||
| - Fixed the `HeaderComponent` margins. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not necessary to fix old changelog records
src/blocks/CardLayout/CardLayout.tsx
Outdated
| import {CardLayoutBlockProps as CardLayoutBlockParams} from '../../models'; | ||
| import {Row, Col} from '../../grid'; | ||
| import {BlockHeader, AnimateBlock} from '../../components'; | ||
| import {HeaderComponent, AnimateBlock} from '../../components'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this renaming? BlockHeader tells more about it's purpose than HeaderComponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me it is confusing that we have HeaderBlock and BlockHeader. I thought that HeaderBlock and HeaderComponent more understandable. How BlockHeader tells about purpose?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BlockHeader tells that it is using for heading in blocks, HeaderComponent not, but raises questions - why do we have both component and block headers?
And HeaderComponent has extra info in its name - we import it form components dir and it is clear that it is a component. And in this case should we add Component prefix to any component?
For me to make things clear with this renaming we could exclude 'header' and use BlockTitle or something like that.
Anyway, we can leave this renaming for major update - it doesn't worth major with itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like BlockTitle, let's leave this for major update. Thanks
| transformer: typografTransformer, | ||
| parser: parseSlider, | ||
| }, | ||
| [BlockType.PromoFeaturesBlock]: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const blockHeaderTransfomer = [
{
fields: ['title'],
transformer: typografTransformer,
parser: parseTitle,
},
{
fields: ['description'],
transformer: yfmTransformer,
}
];
52d0193 to
04639b9
Compare
No description provided.