Navigation Menu

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

Post typography. #146

Merged
merged 3 commits into from Oct 10, 2017
Merged

Post typography. #146

merged 3 commits into from Oct 10, 2017

Conversation

zzap
Copy link
Member

@zzap zzap commented Sep 6, 2017

As per @mapk suggestions:

  • post title restyled
  • reading time moved

I also did some typography fixes for post content and changed markup to get structure closer to one found in other Handbooks.

Screenshot:

screenshot

@zzap zzap self-assigned this Sep 6, 2017
@zzap zzap requested a review from ntwb September 6, 2017 22:38
@joyously
Copy link
Contributor

joyously commented Sep 7, 2017

I don't understand the concept of putting the reading time at the bottom.
It seems like it should be at the top or not at all. In fact, maybe it should be on the archive page by the title link and not on the post itself.

@zzap
Copy link
Member Author

zzap commented Sep 7, 2017

I do agree it's useless below post and that it should be on archives (its position should be another issue, imo).

I moved it at the bottom only because it was requested to be removed from the top and didn't want to remove it completely 😄

@mapk
Copy link
Collaborator

mapk commented Sep 7, 2017

If we want to keep reading time (I don't think it's important for documentation reading), then we should definitely move it back to the top.

I think removing it altogether is perfectly okay. People aren't necessarily reading for pleasure when they get to HelpHub. And technical writings will very in length of reading depending on the proficiency of the person's understanding.

@mrxkon
Copy link

mrxkon commented Sep 16, 2017

I'm on the same spec with @mapk on this. Reading time is always meant to be somewhere along with the title in general. But on 'help' docs is actually either not-important or miss-leading even to some people as they might think ( although not stated anywhere ) that oh hm it says 1 minute so I'll fix it in 2 minutes.

I'd suggest to remove it completely, having time on documents in general seems like more of a stress factor to me than a relief one.

@atachibana
Copy link
Collaborator

Post Title:
Yes, it seems the same with Handbooks. I like this one!

Reading Time:
I have the same opinion with others.
Especially for non-native English speaker, that number is just number. :-)

@zzap
Copy link
Member Author

zzap commented Sep 21, 2017

OK, looks like we decided to remove Reading Time completely. I will update PR.

@mayeenulislam
Copy link

mayeenulislam commented Oct 5, 2017

Style seems good to me, though I's not a listed reviewer. But an issue:

wp4.css is disabling the content's bottom margin for <p> tags inside .entry-content:

wp4

We should fix the precedence of the CSS files. To me, the declaration of wp4.css is kinda normalizing things. We should load it before style.css

@zzap
Copy link
Member Author

zzap commented Oct 5, 2017

To me, the declaration of wp4.css is kinda normalizing things. We should load it before style.css

wp4.css is loaded altogether with wporg header in header.php

It should be loaded before our style.css but that is not possible in this setup. That is the reason I didn't really touch those things which will be inherited once HelpHub is inside .org and order of stylesheets correct.

Most of all, this pull request is about styling post title properly.

@mayeenulislam
Copy link

Most of all, this pull request is about styling post title properly.

The pull request met that part completely and with no conflict, is ready to merge.

float: right;
font-size: 1em;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

With this whole file being removed it should also be removed fro import in style.scss:

https://github.com/Kenshino/HelpHub/blob/master/themes/helphub/sass/style.scss#L97

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

Choose a reason for hiding this comment

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

Ha, I looked twice, my bad, must still be in holiday mode ;)

@zzap zzap merged commit 8f87353 into master Oct 10, 2017
@zzap zzap deleted the post-title branch October 10, 2017 20:48
@zzap
Copy link
Member Author

zzap commented Oct 10, 2017

Fixes #109

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.

None yet

7 participants