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

Multi-echo background documentation edits #351

Merged
merged 10 commits into from
Jul 3, 2019

Conversation

handwerkerd
Copy link
Member

Progress for #290

Changes proposed in this pull request:

  • Created a new page for the embedded publications spreadsheet rather than just having a link to a google doc buried in the middle of a page
  • Put a few more background references and another educational talk into multi-echo.rst
  • Corrected. a few quirks that were causing problems with how the documentation was being formatted for ReadTheDocs

Others should take a look at how the spreadsheet renders within ReadTheDocs to see if they think it's ok. It goes a bit beyond the edge of the standard frame, but I think this still looks better than just asking people to go to a separate webpage with the spreadsheet.

I've explained more about the ReadTheDocs warnings in #350 but I fixed some of the low handing fruit in this PR

Daniel Handwerker and others added 10 commits June 20, 2019 22:21
Co-Authored-By: Joshua Teves <[email protected]>
Small text edits to correctly spell fMRIPrep and add a link to the weighted average function in tedana.
Co-Authored-By: Joshua Teves <[email protected]>
Fixing link to t2smap
# Conflicts:
#	docs/multi-echo.rst
Several references were added to the main text and I split off the
spreadsheet of publications into its own page rather than just linking
to a google doc.
More fixed how publication table looks and corrected a few ‘make html’
warnings for readthedocs
@handwerkerd handwerkerd added the documentation issues related to improving documentation for the project label Jul 2, 2019
@codecov
Copy link

codecov bot commented Jul 2, 2019

Codecov Report

Merging #351 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #351   +/-   ##
=======================================
  Coverage   49.15%   49.15%           
=======================================
  Files          39       39           
  Lines        2142     2142           
=======================================
  Hits         1053     1053           
  Misses       1089     1089

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 173c6d7...1d25f05. Read the comment docs.

@handwerkerd
Copy link
Member Author

This should be a fairly quick review. As @tsalo noted, just run "make html" in the docs directory & you'll be able to see that everything rendered correctly. It would be good if @dowdlelt checked to see if he likes how his spreadsheet of publications is rendering within the documentation. In relation to #350 it would be useful if @jbteves and others also ran "make html" to see what the underlying issue is.

@handwerkerd handwerkerd requested review from jbteves, dowdlelt and tsalo July 3, 2019 14:30
Copy link
Member

@tsalo tsalo left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. The table rendering is really cool.

Copy link
Collaborator

@dowdlelt dowdlelt left a comment

Choose a reason for hiding this comment

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

I think having the spreadsheet embedded is great, even if it looks a little funny being so wide. Feel like that allows people to know what they are getting before the click. Loving all the other resources as well.

In this format, it makes me think that there are some improvements to the overall formatting that would make it look even better. I've fallen far behind on updating details (though I've just added another ~10 or so 2019 papers) but I've not given up - Probably try and clean things up a bit before too long.

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

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

This is fantastic, and I love the spreadsheet embedding! I'm not worried about it being perfect because I'm hard pressed to think of any other software that offers such a comprehensive list of related studies anyway, let alone one embedded so conveniently. Fantastic!
However, I did notice that @dowdlelt is still listed as a grad student, and he is actually now a postdoc : - ) so the spreadsheet should be updated accordingly.

@handwerkerd handwerkerd merged commit 1baed23 into ME-ICA:master Jul 3, 2019
@handwerkerd handwerkerd deleted the DocEdits branch July 3, 2019 18:58
handwerkerd added a commit to handwerkerd/tedana that referenced this pull request Sep 11, 2019
* Adding ME us recommendations to multi-echo.rst

* Small changes to ME us recommendations in multi-echo.rst

* Update docs/multi-echo.rst

Co-Authored-By: Joshua Teves <[email protected]>

* Update multi-echo.rst

Small text edits to correctly spell fMRIPrep and add a link to the weighted average function in tedana.

* Update docs/multi-echo.rst

Co-Authored-By: Joshua Teves <[email protected]>

* Update multi-echo.rst

Fixing link to t2smap

* Merge remote-tracking branch 'upstream/master'

# Conflicts:
#	docs/multi-echo.rst

* Reformated refs in publications.rst

Several references were added to the main text and I split off the
spreadsheet of publications into its own page rather than just linking
to a google doc.

* Additional fixes to documentation

More fixed how publication table looks and corrected a few ‘make html’
warnings for readthedocs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation issues related to improving documentation for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants