Skip to content

Conversation

@crazy-max
Copy link
Member

While working on #15194 I found out there was no proper spacing after the title of some pages:

image

https://deploy-preview-15194--docsdocker.netlify.app/build/bake/

Looking at the layout it seems it was done on purpose to handle the estimated reading time of page so rendering looks right:

image

https://docs.docker.com/desktop/

This PR now keeps the paragraph even if the estimated reading time is not rendered so we are consistent across pages:

image

I also found out that this block was duplicated and available as an include in https://github.com/docker/docker.github.io/blob/f540f58541b02236262f93272790b706c2c48b24/_includes/read_time.html so switch to the include instead.

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@netlify
Copy link

netlify bot commented Jul 26, 2022

Deploy Preview for docsdocker ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 863b227
🔍 Latest deploy log https://app.netlify.com/sites/docsdocker/deploys/62e11bc2a3e0cd000877f3bd
😎 Deploy Preview https://deploy-preview-15218--docsdocker.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, but left one suggestion we could try

<p><em class="reading-time">Estimated reading time: {{ words | divided_by:180 }} minutes</em></p>
{%- endif -%}
{%- endunless -%}
<p>{%- include read_time.html -%}</p>
Copy link
Member

Choose a reason for hiding this comment

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

I guess alternatively, we could add a style for block quotes immediately following a H1;

h1+blockquote {
  margin-top: 24px;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@usha-mandya usha-mandya left a comment

Choose a reason for hiding this comment

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

LGTM

@crazy-max
Copy link
Member Author

Let's get this one in, will see to enhance this with #15218 (comment) in a follow-up.

@crazy-max crazy-max merged commit 43fdab2 into docker:master Jul 27, 2022
@crazy-max crazy-max deleted the p-read-time branch July 27, 2022 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants