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 table of contents to README #435

Merged
merged 3 commits into from
Oct 15, 2019
Merged

Conversation

MohanSha
Copy link
Contributor

@MohanSha MohanSha commented Oct 1, 2019

add table of contents

add table of contents
@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

Merging #435 into master will decrease coverage by 6.96%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #435      +/-   ##
=========================================
- Coverage   78.46%   71.5%   -6.97%     
=========================================
  Files          25      25              
  Lines        1416    1400      -16     
=========================================
- Hits         1111    1001     -110     
- Misses        305     399      +94
Impacted Files Coverage Δ
src/poisson_solvers.jl 40.94% <0%> (-56.72%) ⬇️
src/fields.jl 49.35% <0%> (-15.59%) ⬇️
src/Oceananigans.jl 62.5% <0%> (-12.5%) ⬇️
src/utils.jl 78.26% <0%> (-7.32%) ⬇️
src/time_steppers.jl 72.02% <0%> (-2.1%) ⬇️
src/boundary_conditions.jl 85.24% <0%> (-1.64%) ⬇️
src/output_writers.jl 79.54% <0%> (-0.57%) ⬇️
src/models.jl 82.05% <0%> (+1.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d32cf20...d88b63f. Read the comment docs.

@ali-ramadhan ali-ramadhan changed the title Update README.md Add table of contents to README Oct 1, 2019
@ali-ramadhan ali-ramadhan added the documentation 📜 The sacred scrolls label Oct 1, 2019
@ali-ramadhan
Copy link
Member

ali-ramadhan commented Oct 1, 2019

Hi @MohanSha thank you for this PR! Didn't know you could have a table of contents in the README. This should be useful especially as we add more stuff to the README.

I just have a couple of suggestions:

  1. "Table of contents" feels a little formal to me, I might change the name to just "Contents".
  2. I might also remove * [Oceananigans.jl](#oceananigansjl) as the top-level item in the list. I think it makes more sense to have "Installation instructions", "Running your first model", etc. to be top-level items.

@ali-ramadhan ali-ramadhan reopened this Oct 1, 2019
@ali-ramadhan
Copy link
Member

Woops, sorry I closed the PR by mistake but reopened it.

@ali-ramadhan
Copy link
Member

Hmmm, I also think the movie hyperlinks in the "Contents" should link to sections in the README instead of to YouTube directly.

I guess that would be

[Deep convection](#deep-convection)

instead of

[<a href="https://www.youtube.com/watch?v=kpUrxnKKMjI" rel="nofollow">Deep convection</a>](#deep-convection)

- Changed table of contents to contents
- Made contents top-level items
@ali-ramadhan ali-ramadhan self-requested a review October 15, 2019 20:40
Copy link
Member

@ali-ramadhan ali-ramadhan left a comment

Choose a reason for hiding this comment

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

Thank you for the revisions @MohanSha! Looks great!

@ali-ramadhan ali-ramadhan merged commit eb7fb36 into CliMA:master Oct 15, 2019
arcavaliere pushed a commit to arcavaliere/Oceananigans.jl that referenced this pull request Nov 6, 2019
Add table of contents to README

Former-commit-id: eb7fb36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📜 The sacred scrolls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants