Skip to content

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

Merged
tbouffard merged 12 commits into
mainfrom
187-add_description_in_blog_page
Nov 29, 2021
Merged

[FEAT] Add description in the 'blog' and 'news' pages#226
tbouffard merged 12 commits into
mainfrom
187-add_description_in_blog_page

Conversation

@tbouffard
Copy link
Copy Markdown
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
Copy Markdown

github-actions Bot commented Nov 17, 2021

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

🤖 By surge-preview

Comment thread src/pages/blog.tsx Outdated
@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
Comment thread src/content/NewsContent.tsx Outdated
@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 thread src/theme/pages/PageWithPosts.tsx Outdated
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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