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

Updated README.md and RST Documentation #789

Merged
merged 5 commits into from
Sep 19, 2016

Conversation

robfrawley
Copy link
Collaborator

@robfrawley robfrawley commented Sep 7, 2016

Major refactoring of documentation (both in README.md and RST documentation).

Markdown README

To view how the updated README.md renders, reference robfrawley/LiipImagineBundle/blob/feature-update-docs/README.md.

ReStructuredText DOCS

To view how the updated docs render, reference src.run/docs/symfony_build/liipimaginebundle where I've compiled a build reflective of this pull-request.

The docs were built using a clone of the official Symfony Docs Repo with the required fabpot/sphinx-php python package, as well as the nessissary system binaries. Due to less-than-perfect build-conditions, as well as a lack of documentation as to how Symfony actually performs their documentation compilation, you'll notice that many of the ancillary links in the above referenced build are dead. Don't fret; our only concern is ensuring the links within the liipimaginebundle folder of the build function properly: the links to Symfony and other components are immaterial in this context.

To better understand the doc build handling for the official documentation, reference the _build directory of the official Symfony Docs Repo.

rework of the RST docs and README

cleanup of RST cleanup
@alexwilson
Copy link
Collaborator

This is honestly really great and should help newcomers a lot! 👍

@alexwilson alexwilson mentioned this pull request Sep 9, 2016
3 tasks
@lsmith77
Copy link
Contributor

lsmith77 commented Sep 9, 2016

@javiereguiluz can you have a look?

Create Thumbnails
-----------------

.. _usage-create-thumbnails:
Copy link
Contributor

Choose a reason for hiding this comment

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

In Symfony Docs we usually put the label before the section title. Otherwise, when clicking a link with this anchor, you don't have a clear reference about where you are.

@javiereguiluz
Copy link
Contributor

@robfrawley you did a huge and great work in this PR. I left some minor comments, mostly related to missing double backticks for <code> element. But overall looks OK to me 👍 Thanks!

@robfrawley
Copy link
Collaborator Author

robfrawley commented Sep 9, 2016

Thanks for taking the time to review the PR @javiereguiluz! Very much appreciated. I implemented all your changes, minus the backtick comments, as can be seen in this diff.

As for the backticks, I started using .. default-role:: code about halfway through, as it makes it much easier to copy and paste between markdown and RST when both use single backticks. Problem is, it wasn't standardized because I'd started halfway through writing the docs. As such, I updates all the RST files to use the .. default-role:: code statement at the start and standardized on single backticks. Is this acceptable?

@robfrawley
Copy link
Collaborator Author

robfrawley commented Sep 9, 2016

@lsmith77 After @javiereguiluz okays my last comment, please give me a shout if you want me to squash commits before PR merge. I left the latest commits intact so @javiereguiluz could see a diff of the changes from his comments easily.

@javiereguiluz
Copy link
Contributor

About the default-role ... I'm not sure if ti's going to work. We can merge this and then revert if it's not working ass expected. In any case, all Symfony Docs use the double backticks ... and it shouldn't be hard to do it with a Find + Replace + Regex in your editor 😄

@robfrawley
Copy link
Collaborator Author

robfrawley commented Sep 9, 2016

@javiereguiluz No worries; I've just updated the PR to use double back ticks everywhere to avoid the uncertainty.

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label Sep 13, 2016
@robfrawley
Copy link
Collaborator Author

@lsmith77 With this reviewed by @javiereguiluz, I think it is now ready to be merged! Please advise if you want the PR squashed before merge.

@lsmith77 lsmith77 merged commit cdf007b into liip:master Sep 19, 2016
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label Sep 19, 2016
@robfrawley robfrawley deleted the feature-update-docs branch January 9, 2017 21:12
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