Code review comment for lp:~chris.gagnon/autopilot/added-contribute-documentation

Revision history for this message
Francis Ginther (fginther) wrote :

The contribute page looks very good, I just found a few proofreading corrections:

82 +* Test and Fix: No project is perfect, log some `bugs <https://bugs.launch pad.net/autopilot/+filebug>`_ or `fix some bugs <https://bugs.launchpad.net/autopilot/+filebug>`_.

There are extra spaces in the report a bug link. The second link should just go to the bug list: <https://bugs.launchpad.net/autopilot>

119 +All new features should have unit and/or functional test to make sure someone doesn't remove break your new code with a future commit.

Should that be "remove or break"?

123 +From the directory containing the autopilot source code and list the tests::

Sounds like it's missing something. Maybe "From the directory containing the autopilot source code, you can list the tests"?

The footer has been a lot more complicated then I expected. The original footer is generated by a parameterized sphinx layout.html file (the copyright dates, etc. come from the conf.py file or sphinx itself). We should continue to use the parameters so that the layout.html doesn't need to be changed when those parameters change. For the life of me, I can't figure out from the sphinx docs if there is a way to 'subclass' the original footer and just add the contribute and bug links. So...

The best I could come up with is just a copy and paste of the footer block from /usr/share/sphinx/themes/basic/layout.html:

docs/_templates/layout.html:
{% extends '!layout.html' %}
{% set css_files = css_files + ["_static/centertext.css"] %}

{% block footer %}
    <div class="footer">
      <div class=center_text>
        <a href="{{pathto("faq/contribute") }}">Learn how you can contribute!</a> / <a href="https://bugs.launchpad.net/autopilot/+filebug">File a bug</a>
     <br/>
      </div>
    {%- if show_copyright %}
      {%- if hasdoc('copyright') %}
        {% trans path=pathto('copyright'), copyright=copyright|e %}&copy; <a href="{{ path }}">Copyright</a> {{ copyright }}.{% endtrans %}
      {%- else %}
        {% trans copyright=copyright|e %}&copy; Copyright {{ copyright }}.{% endtrans %}
      {%- endif %}
    {%- endif %}
    {%- if last_updated %}
      {% trans last_updated=last_updated|e %}Last updated on {{ last_updated }}.{% endtrans %}
    {%- endif %}
    {%- if show_sphinx %}
      {% trans sphinx_version=sphinx_version|e %}Created using <a href="http://sphinx.pocoo.org/">Sphinx</a> {{ sphinx_version }}.{% endtrans %}
    {%- endif %}
    </div>

{% endblock %}

Once I did this, I had to remove the background color from div.center_text as the footer class had a slightly different color.

Maybe there is a better way to do this, I'm not very familiar with these templating tools.

Also, I really like the "You know you want to."

review: Needs Fixing

« Back to merge proposal