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 styleguide docs #2008

Merged
merged 27 commits into from Jun 19, 2018

Conversation

Projects
None yet
3 participants
@toolness
Contributor

toolness commented Jun 12, 2018

This attempts to document how to add/change content in our style guide at /styleguide/docs/.

Other notes:

  • It adds and documents a {% template_example %} tag that may be useful in the style guide, to document any Django template-based code snippets that have styling implications.

  • It adds and documents a {% template_tag %} tag that makes it easier to mention our custom template tags and link to their GitHub source.

  • If you load these docs on a local instance, some of the GitHub links will point to the wrong line numbers, because the line numbers are relative to this styleguide-docs branch, while the GitHub links generated by the style guide point to the develop branch. However, in the future, the links will almost always be correct, because the individual files being linked to are changed relatively rarely.

Here's a screenshot of the docs for easy viewing (note that the latest iteration of this PR might have new/changed content, though):

screenshot-2018-6-14 calc calc style guide

toolness added some commits Jun 12, 2018

<div class="sidenav-layout__content">
<div class="card">
<div class="content">
<h1>CALC Style Guide Documentation</h1>

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 13, 2018

Member

I notice the lack of indentation in the calc templates. Was that a decision made somewhere along the way?

This comment has been minimized.

@toolness

toolness Jun 13, 2018

Contributor

Not particularly consciously, but I personally sometimes don't indent because (A) I generally try to keep lines at 80 columns even in HTML and excessive div wrapping makes that hard, and (B) I want git blame/praise/history to stay as accurate as possible, so I prefer not to e.g. indent everything another level when wrapping it in an extra div. But that's just me, and I could go the other way if folks felt strongly about it.

<h1>CALC Style Guide Documentation</h1>
<p>
Welcome to the CALC Style Guide Documentation! Here you'll find guidelines on how to update the <a href="{% url 'styleguide:index' %}">CALC Style Guide</a>.

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 13, 2018

Member

Personally, I think we can strike the welcome sentence. I'm not sure it adds any value, and I should know where I just navigated to.

This comment has been minimized.

@toolness

toolness Jun 13, 2018

Contributor

The one advantage of the link is that it can take users back to the styleguide if they've just clicked on a link that takes them to these docs. That could also be done with some kind of breadcrumb though.

Also, though, this is just a WIP and I wanted to get something started, so the content here isn't at all ready for review yet :)

</p>
{% guide_section "Template tags" %}
<p>

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 13, 2018

Member

it seems odd to me that we'd say they exist but not document what they are or how to use them.

This comment has been minimized.

@toolness

toolness Jun 13, 2018

Contributor

Yep, I'm planning on that--as mentioned earlier, this is just a WIP right now and I'm just pushing early, pushing often as I iterate on this stuff.

@@ -26,6 +26,34 @@
register = template.Library()
@register.tag
def template_example(parser, token):

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 13, 2018

Member

This looks to me like a place where an inclusion tag would be about perfect, and much, much easier to read. You might even get by with a simple tag or assignment tag.

This comment has been minimized.

@toolness

toolness Jun 14, 2018

Contributor

Hmm, how would you do that? I guess I don't know of any other way to get both the rendered content between tags and the original source of that content...

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 14, 2018

Member

I just have a pretty strong distaste for the old-style two-part template tags at this point. I think they're really arcane and difficult to follow, where the more modern inclusion tags, assignment tags and simple tags are really just simple functions. Or, in the case of an inclusion tag, really sort of a mini-view. For an inclusion tag here, I think it'd be about the same, really. Same args, same code, more or less, just a lot more readable.

This comment has been minimized.

@toolness

toolness Jun 14, 2018

Contributor

Hmm, I want to be able to have the template example be included inline in the parent template, though, and I don't understand how to do that without having an opening and closing tag that demarcates where the example starts and ends.

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 14, 2018

Member

It's entirely possible I'm just not understanding exactly what's going on here. Can you clarify what exactly your passing into it and what you want the output to be. I think it takes a path to a template then outputs that template HTML with some surrounding text. Is that right?

Assuming my reading above is correct, here's a rough and untested take on how it might work: https://gist.github.com/tbaxter-18f/ff92e03a4b97dc4fa8e7775b3707aba7

This comment has been minimized.

@toolness

toolness Jun 14, 2018

Contributor

Oh, it shows both the template source and the rendered template output in an "example" div. For instance, the following:

{% template_example %}
See {% pathname "README.md" %} for some basic info.
{% endtemplate_example %}

produces the example snippet shown at the bottom of the screenshot below:

@@ -17,6 +17,10 @@ def test_styleguide_returns_200(self):
response = self.client.get('/styleguide/')
self.assertEqual(response.status_code, 200)
def test_styleguide_docs_returns_200(self):
response = self.client.get('/styleguide/docs/')
self.assertEqual(response.status_code, 200)

This comment has been minimized.

@tbaxter-18f

tbaxter-18f Jun 13, 2018

Member

I know it would be somewhat susceptible to breakage if the template changed, but what do you think about a test that the template tag actually included something expected from the template?

This comment has been minimized.

@toolness

toolness Jun 13, 2018

Contributor

Good idea, I will add some extra tests!

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 13, 2018

Is this aimed more at developers or non-developer-type users. If the former, could we reach the same goal with the built-in Django admin docs? If the latter, are we adequately addressing their needs.

(And yes, I do have a "Persona-Driven Documentation" talk proposal already drafted)

@toolness

This comment has been minimized.

Contributor

toolness commented Jun 13, 2018

The personas here are @hbillings and @ericronne who were confused about how to add/change styleguide content about a month ago... I had to show them the ropes in a video call, so I figured it would be good for the long-term if it were documented in an easier-to-consume way.

We actually have enabled admindocs in #1447 but have lots of issues with it. Frankly I think the documentation will be a lot easier to read if we can include it as part of the styleguide and have more control over its presentation than via admindocs, which is fairly rigid and allows for very little customization (see that issue for some of our quibbles with it).

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 13, 2018

I'm leaning toward thinking this should just be handled in admin docs. Other than some presentational issues that appear to be pretty easily resolved, I'm not seeing the quibbles. It's possible there's additional context I'm missing, though. This feels like we're inventing a new way to do something we can already do. What am I missing? I do think the admin docs are very developer-centric and if you're after more end-user-centric documentation they may not be appropriate, but I don't think that's the case here. Full disclosure: I think the built-in admin docs are criminally under-utilized by most projects and can be wildly useful.

@toolness

This comment has been minimized.

Contributor

toolness commented Jun 13, 2018

Here's an example of what this looks like in action:

screen shot 2018-06-13 at 12 41 44 pm

Things to notice:

  1. The {% pathname %} links to the line number where the template tag function is defined on GitHub, so it's easy to see the source code. (Additionally, an assertion will be thrown if the template tag doesn't exist, which prevents doc rot.)

  2. The example shows both the original Django template source and what it looks like when rendered. This isn't just useful for styleguide-related template tags; it will also be useful for template tags which have a significant stylistic component, which is one of the major reasons this is preferable over the built-in admindocs functionality. Folks who are using such template tags are going to know what they actually look like when rendered, which admindocs doesn't do AFAIK.

  3. Similarly to the style guide itself, the rendered examples are also useful as quick manual tests to verify that our template tags are doing what we think they should be doing. They also serve as basic smoke tests.

  4. Viewing this doesn't require logging in as admin, which makes it very easy to share with TTS folks on Slack. In contrast, for some reason admindocs is behind admin auth and it's non-trivial to separate from that.

@tbaxter-18f

This comment has been minimized.

Member

tbaxter-18f commented Jun 13, 2018

Well, here's my issues with this (at least, off the top of my head).

  1. You're splitting your documentation.
  2. If you're not splitting and intend to fully replace the admin docs, then you're taking on the load of completely reengineering and implementing something django gives you for free. Which is a non-trivial commitment.
  3. The issues you're seeing on the admin docs are largely self-inflicted, due to prior decisions to reskin the admin -- which is itself a non-trivial task with a lot of different cases to consider -- and by the decision to constrain the width to 980px. I think you'll get a lot more mileage out of putting resources into addressing those underlying issues than by re-engineering the docs section.

Regarding your points above:

  1. That's a nice addition, but anyone familiar with django apps at all already knows exactly where to find the template tags. They're always in the same place, and in the admin (if the admin isn't broken) they're already broken out by the app in which you'd find them. So while I think it's nice, personally I'd never use it because if I'm looking at the developer documentation, I already have the codebase open in front of me, too, and I already know where to see the source of it should I want to.
  2. I think all of the tags should be properly commented and documented in the source, which would include how to call the tag, and the template code it would output. Done properly, that would then appear in the admin docs, too. I do, however, think that some sort of render_tag functionality would be a nice addition/extension to the admin... especially in a case like you have, where the admin is using the site CSS and it should be predictable.
    3 is much like (2). I think that would be a worthy addition.
  3. This raises the question: why exactly would you want to share with people? What problem are you solving there? While I can see why the decision to put the admin docs in admin may rankle, in practice I've never seen it be an issue because those docs are aimed at developers, all of whom already have acess to the admin, at least at the local level. But I also understand there may be another problem you're trying to solve there.

For me, this is just looking like a fairly heavyweight solution to a largely-solved problem brought on by some existing issues that should probably be addressed anyway. That's not a comment on the solution itself, which I think is extremely well done.

@toolness

This comment has been minimized.

Contributor

toolness commented Jun 13, 2018

  1. To each their own, I guess? I personally find the links to source extremely useful and use them a lot.
  2. I don't like adding examples in docstrings if they can't be doctested. Otherwise it's extremely likely that they'll lead to doc rot. Furthermore, the output of template tags is usually HTML, not text, so actually displaying the rendered content (perhaps with another tab allowing the rendered HTML to be shown as well) is ideal.
  3. We've shared the styleguide and Sphinx documentation tons of times for folks in TTS (and even on the wider internet) when conversations happen to lead in directions that make the knowledge sharing useful.

I should also note that I've used admindocs before and there's things about it that confuse me and might be bugs. In short, I am pretty passionate about having good documentation and to me the benefits of having really solid, user-friendly documentation with working, up-to-date examples is worth the effort, particularly with respect to what's currently considered state-of-the-art in Django, which I'm quite underwhelmed by.

@hbillings

This comment has been minimized.

Member

hbillings commented Jun 13, 2018

So it strikes me that this is a good debate, but this is probably the wrong place to have it -- it seems like it would be better to open an issue related to this PR and move the discussion there. I say this because we're supposed to be in a feature development sprint right now, and anything related to the styleguide seems to fall pretty clearly in housekeeping sprint territory, so we should circle back to this then (our next one is in two weeks).

I also think that spending cycles redoing what we've got is fairly low priority, given the massive backlog of features we've got. If there's a way to easily document that without reworking how stuff currently builds, we can move the discussion about whether we should rebuild it to an issue we can discuss during housekeeping.

@toolness toolness changed the title from [WIP] Add styleguide docs to Add styleguide docs Jun 14, 2018

toolness added some commits Jun 15, 2018

@hbillings

Super helpful info -- did we ever file an associated issue about revisiting structure?

// WebComponentLink where a web component's source code
// is located. It needs to be defined on the prototype of
// every web component that's pointed at by a
// WebComponentLink, or else a JS alert will be raised.

This comment has been minimized.

@hbillings

hbillings Jun 19, 2018

Member

ooooh okay

@toolness

This comment has been minimized.

Contributor

toolness commented Jun 19, 2018

Yay thanks! I don't think we filed such an issue about revisiting structure yet...

@toolness toolness merged commit 3024c0d into develop Jun 19, 2018

3 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codeclimate/total-coverage 92% (0.0% change)
Details

@toolness toolness deleted the styleguide-docs branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment