Skip to content
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

Add support for comments via utterances #65

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

opeik
Copy link

@opeik opeik commented Jul 20, 2024

Hi there,

This PR adds support for comments via utterances.

Demo

demo

sass/buttons.scss Show resolved Hide resolved
@@ -1,3 +1,8 @@
html {
/* Base font size */
font-size: 16px;
Copy link
Author

Choose a reason for hiding this comment

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

Added an explicit base font size since everything else uses rem, it was already implicitly 16px.

@@ -29,8 +29,6 @@ body {
}

h1, h2, h3, h4, h5, h6 {
display: flex;
align-items: center;
font-weight: bold;
Copy link
Author

Choose a reason for hiding this comment

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

display: flex breaks how inline code blocks are rendered in headings, so I removed it.

@@ -118,7 +116,6 @@ figure {
}

code {
font-family: Hack, DejaVu Sans Mono, Monaco, Consolas, Ubuntu Mono, monospace;
Copy link
Author

Choose a reason for hiding this comment

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

Moved the font family to the font file.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm, I don't see this font-family anywhere now. Are you sure it won't break things?

Copy link
Author

Choose a reason for hiding this comment

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

It's defined here. I've tested the changes locally but it doesn't hurt if you give it a try to sanity check.


&__title {
display: flex;
text-align: center;
position: relative;
margin: 100px 0 20px;
margin: 25px 0 20px;

Copy link
Author

Choose a reason for hiding this comment

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

I adjusted the spacing between the comments and pagination sections so it looked nicer.

Copy link
Owner

@pawroman pawroman left a comment

Choose a reason for hiding this comment

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

Hey, thanks for the contribution! TIL about utterances. Quite cool!

Please address the comments, as well as my comment from other PR about the TOC #68 (comment) -- because it also applies here.

After the outstanding comments are resolved, I'll need to review it again for visual changes. But overall looks good!

sass/buttons.scss Show resolved Hide resolved
@@ -118,7 +116,6 @@ figure {
}

code {
font-family: Hack, DejaVu Sans Mono, Monaco, Consolas, Ubuntu Mono, monospace;
Copy link
Owner

Choose a reason for hiding this comment

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

Hmmm, I don't see this font-family anywhere now. Are you sure it won't break things?

sass/style.scss Outdated Show resolved Hide resolved
@opeik opeik requested a review from pawroman August 24, 2024 02:45
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.

2 participants