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

Blog category updates #435

Open
wants to merge 2 commits into
base: gh-pages
Choose a base branch
from

Conversation

iamchrissmith
Copy link
Contributor

@alicetragedy I took a look at those other two issues and found a fix for both.

For the first, I assumed you meant someplace in the sidebar, so I moved the category listings there. They still link to the categoryview anchor tags, but will allow users to quickly navigate to posts just in a category.

For the comma issue... yeah I agree that is a weird one. I'm not sure where that space is coming from. After investigating, it looks like it is actually helping us for the 'and' one, so I didn't want to nuke it (even if I could). I ended up adding a css hack (margin-left:-2px) for the comma which seems to have corrected the issue at least visually.
Hope these are helpful!
Chris

@alicetragedy alicetragedy self-requested a review October 28, 2017 14:06
Copy link
Member

@alicetragedy alicetragedy left a comment

Choose a reason for hiding this comment

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

Apologies it took me so long to review this again. The sidebar changes look great so I'd keep those, but remove the “comma hack”. I've made more detailed comments in the code itself. I'm also going to leave a separate comment regarding what I originally meant with “the categories can't be seen on the blog index” with screenshots so that it's clearer. Thank you so much for all your work so far!

@@ -36,7 +36,22 @@ <h3 class="l-sidebar__heading">Latest posts</h3>
<li><a href="{{ post.url }}">{{ post.title }}</a></li>
{% endfor %}
</ul>
<p>See all posts in the <a href="/blog/archive/">archive</a> or <a href="/blog/categoryview">browse by category</a>!</p>
<p>See all posts in the <a href="/blog/archive/">archive</a> <!-- or <a href="/blog/categoryview">browse by category</a>!--></p>
Copy link
Member

Choose a reason for hiding this comment

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

This looks great! Thanks 👍

@@ -99,3 +99,7 @@
width: 200px
}
}

Copy link
Member

Choose a reason for hiding this comment

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

I think this comma thing is a little too much of a hack, so I would remove it 😅 and just hope we can figure out how to fix it in the future. I also don't think we have the use case very often where a blog post will belong to more than two categories. I really appreciate the effort, Chris!

@alicetragedy
Copy link
Member

I've made some screenshots (see below) of what I meant originally when I said that you couldn't see the categories on the blog index. Basically, when you go on /blog, you see the last few posts, and the information under the title only shows post date and author. The categories of each blog post are only visible in the “single blog post view” (basically, when you click on the post to view it by itself). I'm not sure if this is a big deal or not, what do you think? It was something that confused me originally but I'd love to have your thoughts on this.

view from the blog index
screen shot 2017-11-17 at 21 20 35

view from the single blog post
screen shot 2017-11-17 at 21 20 59

@alicetragedy
Copy link
Member

Hej @iamchrissmith, would you be able to and have time to pick up this PR again? There's a few changes I requested which I can help with, and I'd love to see this merged soon!

@iamchrissmith
Copy link
Contributor Author

@alicetragedy I have been meaning to get back to this, but have been diverted by holidays and a new job. I have travel and a move over the next month or so. I would still be happy to get back to this, but it might be a couple of months. Sorry for the delay!

@alicetragedy
Copy link
Member

no worries, @iamchrissmith! Totally understandable. I'll put this on the back burner for now and ping you about it again in a few weeks.
In the event that we need this merged earlier, would it be ok for you if we cherry-picked your commits (so your contributions would still show) and built on top of what you already have? I'll see how to proceed, but I wanted to ask you just in case!

@iamchrissmith
Copy link
Contributor Author

Of course, please cherry pick away, don't let me become a blocker :-)

@alicetragedy
Copy link
Member

@iamchrissmith sorry for following up so late :) Is this something you'd be able to and be interested in improving? I'm not sure what your time commitment looks like over the next weeks and if you are able to look into this :)

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