Skip to content

[FEAT] Add description in the 'blog' and 'news' pages #226

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

Merged
merged 12 commits into from
Nov 29, 2021

Conversation

tbouffard
Copy link
Member

@tbouffard tbouffard commented Nov 17, 2021

covers #187

@tbouffard tbouffard added enhancement New feature or request WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft labels Nov 17, 2021
@github-actions
Copy link

github-actions bot commented Nov 17, 2021

♻️ PR Preview e4453ee has been successfully destroyed since this PR has been closed.

🤖 By surge-preview

@tbouffard tbouffard force-pushed the 187-add_description_in_blog_page branch from b65fa5a to a8e4813 Compare November 22, 2021 15:28
@csouchet csouchet force-pushed the 187-add_description_in_blog_page branch from a8e4813 to 6c52840 Compare November 22, 2021 16:22
@tbouffard tbouffard force-pushed the 187-add_description_in_blog_page branch from 1f69dc6 to 57bb415 Compare November 26, 2021 11:27
@tbouffard tbouffard changed the title [FEAT] Add description in the 'blog posts' page [FEAT] Add description in the 'blog' and 'news' pages Nov 26, 2021
@tbouffard tbouffard marked this pull request as ready for review November 26, 2021 11:28
@tbouffard tbouffard removed the WIP 🚧 Pull request in progress and/or not ready for review. Used in addition to marking it as draft label Nov 26, 2021
Comment on lines 56 to 67
<Heading
as="p"
color="primary"
fontSize={[2, 4]}
mb={[3, 5]}
textAlign="center"
style={centerHorizontally}
>
<Text width={[400, 600, 700]} key="presentation">
{description}
</Text>
</Heading>
Copy link
Member

Choose a reason for hiding this comment

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

There is a problem. The cards take the same witdh that this text.
image

image

Copy link
Member

Choose a reason for hiding this comment

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

Also, I would prefer to simplify this code and the rendered objects in the DOM, like:

Suggested change
<Heading
as="p"
color="primary"
fontSize={[2, 4]}
mb={[3, 5]}
textAlign="center"
style={centerHorizontally}
>
<Text width={[400, 600, 700]} key="presentation">
{description}
</Text>
</Heading>
<Text
as="p"
width={['85%', '80%', '70%']}
color="primary"
fontSize={[2, 4]}
mb={[3, 5]}
textAlign="center"
style={centerHorizontally}
>
{description}
</Text>

It also is recommended to use 'rem' or '%' in place of 'px' in CSS.
It prevents some effect like:

px %
image image

Copy link
Member Author

Choose a reason for hiding this comment

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

I am ok for using percentage instead of other configuration. I took the width values from existing we had from the site, so this should be applied everywhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

done in e4453ee

Copy link
Member

Choose a reason for hiding this comment

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

In a future PR, I'll change all the unit for the different CSS styles 😉

Copy link
Member

@csouchet csouchet left a comment

Choose a reason for hiding this comment

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

@tbouffard tbouffard requested a review from csouchet November 29, 2021 14:50
@tbouffard tbouffard merged commit 20329c4 into main Nov 29, 2021
@tbouffard tbouffard deleted the 187-add_description_in_blog_page branch November 29, 2021 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants