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 a table of contents to the blog posts #10

Merged
merged 19 commits into from
Jun 27, 2020

Conversation

xvallspl
Copy link
Contributor

@xvallspl xvallspl commented Jun 16, 2020

Added a prototype for a post's table of contents

Still missing:

  • Make it optional

  • Move changes in css to styles

  • Maybe make the table of contents available in the menu on mobile? or collapsable?

  • Improve design (I am not the right person for this)

@xvallspl
Copy link
Contributor Author

I just realized I edited the css directly instead of through stylus (never heard of it before 😅). Do you want me to change that? I see that this is a question you ask in Issue #2 but I don't know how to answer, web development is not my thing and I haven't been initated to any of the shiny stuff with their bells and whistles :)

@jakewies
Copy link
Owner

Hmmm.

I would definitely prefer not touching the css directly. We should only be touching the source code, which lives in src/styles. The styles are written with stylus.

However, the asset pipeline for this project has not been implemented fully since I transitioned my site to a theme. Tomorrow I will spend some time thinking about how to get that up and running. I'll document how to edit the styles in a CONTRIBUTORS.md file. This should give you a good landscape to work with.

That would be a blocker for this PR.

@xvallspl
Copy link
Contributor Author

OK, thanks!

@jakewies
Copy link
Owner

jakewies commented Jun 17, 2020

Hey @xvallspl,

I updated the asset pipeline in a PR earlier this morning. I also added a CONTRIBUTING.md file to help onboard contributors such as yourself! 😄

Here's the rundown for css:

  1. Each page has a corresponding css file made up of unique styles for that specific page and partial css for other common use cases.

  2. The source for these styles is in src/styles. They are written in stylus.

  3. To make changes to css, make sure you are watching the assets so gulp can rebuild the static directory on any changes.

A good suggestion for developing this theme is to:

  1. Run yarn develop in one terminal window. This will serve the example site.
  2. Run yarn watch:assets in a second terminal window. This will trigger a rebuild of the static directory when any changes have been made.

NOTE: The static directory is not playing nice with git. For reasons I'm not yet sure of, git is constantly marking all css files in the static directory as "modified". It might have something to do with the sourcemaps.

@xvallspl
Copy link
Contributor Author

Thanks!

@xvallspl
Copy link
Contributor Author

Hey @xvallspl,

I updated the asset pipeline in a PR earlier this morning. I also added a CONTRIBUTING.md file to help onboard contributors such as yourself! 😄

Here's the rundown for css:

  1. Each page has a corresponding css file made up of unique styles for that specific page and partial css for other common use cases.
  2. The source for these styles is in src/styles. They are written in stylus.
  3. To make changes to css, make sure you are watching the assets so gulp can rebuild the static directory on any changes.

A good suggestion for developing this theme is to:

  1. Run yarn develop in one terminal window. This will serve the example site.
  2. Run yarn watch:assets in a second terminal window. This will trigger a rebuild of the static directory when any changes have been made.

NOTE: The static directory is not playing nice with git. For reasons I'm not yet sure of, git is constantly marking all css files in the static directory as "modified". It might have something to do with the sourcemaps.

From what I can see, the reason why git is marking them as modified is because they are. The files are missing linebreaks now. Why does that happen, I don't know yet 🙃

@jakewies
Copy link
Owner

My first thought is that it has something to do with prettier. I'll do some more digging.

@jakewies
Copy link
Owner

Hugo has a ToC document that could be helpful. Not sure if you are using it or not 👍

@xvallspl
Copy link
Contributor Author

Yes, that's what generates the ToC automatically

@jakewies
Copy link
Owner

@xvallspl just peeped your site and I love what I'm seeing with the ToC <3

@xvallspl
Copy link
Contributor Author

Thanks! I just reworked the commits so the changes are applied in stylus and not in the css and I added an opt-in parameter to it

@xvallspl
Copy link
Contributor Author

@jakewies I'm not sure having the ToC on mobile is one of my priorities right now. Maybe we can add it as an issue for later or for someone to take. As for making everything prettier, can I ask for your help? In that case we could merge as soon as you are happy with the changes and I applied your feedback

@jakewies
Copy link
Owner

Agreed. We can hide on mobile for now. I’ll def help out where needed. Let me know when you’ve wrapped up your work on this branch and I’ll hop in.

@xvallspl
Copy link
Contributor Author

You can hop in now if you want

@xvallspl xvallspl force-pushed the TableOfContents branch 2 times, most recently from 43a7f17 to ed38dd6 Compare June 20, 2020 10:33
@xvallspl
Copy link
Contributor Author

Somewhat painfully rebased to the new layout structure.

@jakewies
Copy link
Owner

Somewhat painfully rebased to the new layout structure.

Sorry about that! 😅

@kentnek is making some changes to the asset pipeline. We'll be switching away from stylus. It'll be good for this project in the long run, but I know it puts some strain on your work for the moment. We'll get this feature in the project soon though. I think it will be a great addition!

@xvallspl
Copy link
Contributor Author

I actually appreciate his changes as I had something like that on the wishlist 😁

@jakewies
Copy link
Owner

@xvallspl Sorry I've neglected this PR a bit. What's the status? Is it time for me to hop in and take a look at the styles?

@xvallspl
Copy link
Contributor Author

I'll adapt it to the scss first!

@jakewies
Copy link
Owner

I've seen it too. It's buggy for sure. I'm not 100% sold that it's the markdown page but it could be. The ToC is getting visibility: hidden at 1024px and below, and normally this could be accomplished with display: none but because we're acting upon the ToC with JS, we can't use display: none. When manipulating the screen size I think there is some bugginess that might be attributed to the css style.

@kentnek
Copy link
Collaborator

kentnek commented Jun 25, 2020

I know why. It's because of the table in the middle of the page.

I've seen websites making tables horizontally scrollable when they're too wide, but I'm not sure how to do that (other than specifying overflow-x: auto, which seems not to work in this case). Could you help with that when you're free?

Other than that I think the PR is good to go, what's left is rebasing and adding some docs on the TOC config.

@jakewies
Copy link
Owner

jakewies commented Jun 26, 2020

@kentnek how does the site param showPageTitleInTOC work? I tried adding it to config.toml but it has no effect.

EDIT: I realized I wasn't putting the value underneath [params]. Whoops 😅

Should we just wipe this option and default to showing the title in the ToC at this point? I feel like the use case is so small that we don't need to support it unless there is enough demand for it.

@jakewies
Copy link
Owner

The last thing that needs doing, as @kentnek said in his previous post, is to document the usage of the ToC. This is a good time to discuss post frontmatter and general configuration of this theme.

I think it's best to be as explicit as possible to avoid confusion, so my proposal is that we update the blog archetype with all possible frontmatter that exists for a blog post. The archetype will include the default values for each blog post, and users can change them on a post-by-post basis.

In my opinion this is a better solution than letting users update post configuration in the site params via config.toml. Yes it requires a few more keystrokes but it's very explicit.

The example posts can all be set to toc: false except for one of them to show off how the ToC looks. I will add a commit to include these changes and some docs in the README and then if everything looks good to you guys we can merge!

@jakewies
Copy link
Owner

Ok documentation is updated and all that's is to have you guys do a once over on everything before I rebase with master and merge this puppy.

@kentnek
Copy link
Collaborator

kentnek commented Jun 26, 2020

Should we just wipe this option and default to showing the title

Oh I actually want to not show the title by default 😆 So I figured just make it an option so that I don't need to override the layout later on.

Other than that and the wide table bug, the PR is good now.

@kentnek
Copy link
Collaborator

kentnek commented Jun 27, 2020

@jakewies @xvallspl shall we merge this PR today?

@jakewies
Copy link
Owner

@kentnek Yup I have some time this morning to take care of it. I'll handle the conflicts as well.

@jakewies
Copy link
Owner

Hey @xvallspl, thanks a ton for your work on this 🙌

Maybe make the table of contents available in the menu on mobile? or collapsable?

We can discuss this in a future PR. For now I'm going to merge this and cut a new release of the project. This was a fun feature to work on with you and @kentnek !

@jakewies jakewies merged commit 1040745 into jakewies:master Jun 27, 2020
@jakewies
Copy link
Owner

@all-contributors please add @xvallspl for code, design

@allcontributors
Copy link
Contributor

@jakewies

I've put up a pull request to add @xvallspl! 🎉

@jakewies jakewies changed the title [WIP] Add a table of contents to the blog posts Add a table of contents to the blog posts Jun 27, 2020
@jakewies jakewies mentioned this pull request Jun 27, 2020
@xvallspl
Copy link
Contributor Author

@jakewies @kentnek Thanks for everything guys! I'm sorry I haven't been able to follow this for the last days.

Now I only have a couple of issues with the result: the fact that the post is not centered in the screen (I guess this is by design, as it wasn't before either), and that I see the table of contents conceptually belonging to the post. I find it out of place so far away from the post in a big screen. A couple of screenshots:

Annotation 2020-06-28 115922

Annotation 2020-06-28 115158

Although this might be only me, I would like to hear your thoughts on it.

@xvallspl
Copy link
Contributor Author

Thoughts? @jakewies @kentnek

Sorry I didn't look at this for weeks

@kentnek
Copy link
Collaborator

kentnek commented Jul 13, 2020

^ yeah I actually had the idea of moving it a bit closer to the actual contents, so the whole (content - TOC) block is centered instead. Also for mobile the TOC can be right after the main title (or the <!--more--> hook, although I'm not sure how to achieve that yet).

However I recently found a layout bug that affects contents that are too wide on mobile (and fixed that on my own fork), so I will deal with the TOC again once I finish fixing that.

@xvallspl
Copy link
Contributor Author

Thanks @kentnek !

@SanchithHegde
Copy link
Contributor

Hey, I noticed that the site param showPageTitleInTOC (introduced in this commit) hasn't been documented yet. Should I include it in the example site config and the README and create a PR?

@jakewies
Copy link
Owner

jakewies commented Aug 7, 2020

Hey @SanchithHegde that would be awesome if you.

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.

4 participants