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

Port ODK 2.0 documentation into Sphinx #480

Merged
merged 22 commits into from Mar 13, 2018

Conversation

Projects
None yet
8 participants
@jbeorse
Member

jbeorse commented Feb 1, 2018

addresses #436

It also addresses the steady stream of forum posts expressing confusion at the status of the ODK 2.0 tools and their documentation. Here is the most recent one: https://forum.opendatakit.org/t/about-odk-2-0-tools/11588/2

What is included in this PR?

I have moved the entirety of the ODK 2.0 documentation into this repository. I have ported the docs into reStructuredText and applied the ODK style guidelines. This site was my source, but I also did some minor updating of the text to fix errors as I found them, remove release designations (the suite is now fully released), and break up the pages into a similar structure as the rest of the docs.

I have added a fairly unobtrusive link to the ODK 2.0 docs to the home page. But I imagine this will be a point of discussion, that this language is particular, and I intended this only as a suggestion. I will update this to whatever structure the community agrees on.

What is left to be done in the addressed issue?

The purpose of this PR is to migrate the ODK 2.0 docs into the main documentation website. However after this merge I will continue improving and adding to the ODK 2.0 docs. In particular there are community made tutorials that will be integrated.

This PR also does not complete the migration process from the old website. The Google Doc linked in issue #436 has a list of more documentation that needs to be addressed.

What problems did you encounter?

  1. Unfortunately this PR is quite large. I am not aware of a way to break it up into smaller pieces because the various ODK 2 doc pages all reference each other.

  2. I attempted to use the new style-test.py before submitting, but the script crashed.

@ankita240796

This comment has been minimized.

Collaborator

ankita240796 commented Feb 1, 2018

Hi @jbeorse, Sorry for the issue here. The test fails because index is not checked before accessing the character at that position. I have issued a PR #482 to fix that. Once that is merged, the build can be restarted. Also you can apply those changes locally to the style-test script and then run it. That will fix the issue!

@jbeorse

This comment has been minimized.

Member

jbeorse commented Feb 1, 2018

Thanks @ankita240796! I'll check that out locally and start cleaning up any errors I find in my docs.

.. note::
Whenever you are developing an application, and have found a need to add a new column to an existing table, you will need to manually delete the data tables from the server before using the :guilabel:Reset App Server` button from the device.

This comment has been minimized.

@ankita240796

ankita240796 Feb 1, 2018

Collaborator

You have left a backtick here in the :guilabel: role due to which the test fails. Please fix that!

@jbeorse

This comment has been minimized.

Member

jbeorse commented Feb 1, 2018

Thanks again. I fixed the error you pointed out, as well as two spelling errors caught by the script. Now when I run it I see 0 errors.

I do seem to have upped the warnings from 543 to 724.

Effective Access
-----------------------
As mentioned above, when a SQL query is processed inside the ODK Services layer, it is first examined to see if the result set contains the columns :th:`_sync_state, :th:`_default_access`, :th:`_row_owner`, :th:`_group_read_only`, :th:`_group_modify`, and :th:`_group_privileged`. If it contains all six columns, then a synthesized column, :th:`_effective_access` is added to the result set. That column returns one of *r*, *rw*, *rwd*, or *rwdp* (with the *p* indicating that a user can change permissions for the row as well) to indicate the level of access the current user has on the rows in the result set.

This comment has been minimized.

@ankita240796

ankita240796 Feb 1, 2018

Collaborator

Missing backtick in :th:`sync state

This comment has been minimized.

@jbeorse

jbeorse Feb 1, 2018

Member

Thanks, just pushed a fix.

@ankita240796

This comment has been minimized.

Collaborator

ankita240796 commented Feb 1, 2018

@jbeorse Try running on the files you have changed in the following way:
python style-test.py [file1] [file2]...
You can also generate a csv output for errors:
python style-test.py -o out.csv [file1] [file2]...
It will make it easier for you to see the warnings only in the files changed by you. There are many false positives as well.

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Feb 1, 2018

This might raise the relative priority of moving the rst files into a src subdirectory...

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Feb 1, 2018

I do seem to have upped the warnings from 543 to 724.

GH says this PR has 92 files in it, so your warnings-per-file rate is much lower than what we have already.

@jbeorse

This comment has been minimized.

Member

jbeorse commented Feb 2, 2018

@adammichaelwood I agree that the .rst files everywhere are a bit of a mess, and my PR certainly doesn't help with the clutter. I left everything at root level to be consistent with the current style, but will happily move them into a more organized folder structure.

Is there an open issue describing what the move should look like?

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Feb 2, 2018

Is there an open issue describing what the move should look like?

Tentatively, #401

Basically, I just want to move all the rst files, and conf.py, into a dir called src (or something) and all the various and increasing python authoring and build utilities into util. That would at least keep things moderately organized. I wouldn't mind topical subdirectories in theory if they didn't effect output URI

@jbeorse

This comment has been minimized.

Member

jbeorse commented Feb 6, 2018

I've opened PR #496 to address Issue #401 . If/when that gets merged I'll update this PR to follow suit.

@jbeorse

This comment has been minimized.

Member

jbeorse commented Feb 9, 2018

Since PR #496 has been merged I've updated this PR with those changes, along with the style guide fix in PR #501. This should be ready to go again.

@lognaturel

This comment has been minimized.

Member

lognaturel commented Feb 9, 2018

First, this represents a ton of work and looks really fantastic. Thank you @jbeorse.

The placement right now is a bit odd because the "developing with ODK", "integration" and "reference" sections all focus on 1.0 concerns. Did you consider the option of an analogous but separate docs site? I'm wondering whether that might give each doc set more of an opportunity to be a first class citizen. The website could then link out to the two doc sets side by side. This would also make doc search more useful so that users don't have to very carefully note what suite each page is for (e.g. search for "calculation" and it's not clear which results are for 1.0 tools and which are for 2.0 tools).

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

I've reviewed this, and I think this is going to be the best way to move forward.
From my perspective, there's nothing to stop this from being merged in its present state ---
assuming everything works from a deployment standpoint.

There's a few things I'd like to work out, but they can be done moving forward:

  • At present, there's not a sensible way for shared-src documents to reference other pages.
    This PR removes a number of links to odk1 pages in the contributors guide,
    and then other links are treated as external links with full URLs.
    I'd prefer to some way for us to be able to link to either docset from shared docs.
    I suspect this would involve using intersphinx and either some build-time regex replace
    or a custom role.

  • The template files are in shared-src, which is fine.
    But as long as we're doing all this build-time file shifting,
    I'd love to see all that extracted out so that src directories only contain content.

  • I'm not sure if FAQ needs to be shared. At least, at the moment it probably doesn't.

  • Likewise with the Glossary. Maybe we need separate Glossaries? Or maybe we need little tags or something to differentiate ODK1 and ODK2 terms...

  • Some of the "ODK1 vs. ODK2" content should probably be shared. It doesn't have to be in the same place, BTW. It could be in the front matter on ODK2, like it is now, and in Reference in ODK1.

  • This has been mentioned before, but --- now that I'm seeing it in two tabs on a browser ---
    There really does need to be some visual difference between the two doc builds.
    An ODK2 logo would help (is there one?), and some language in the header.
    But that wouldn't be enough.
    We need something for those of us who will be regularly dealing with both builds,
    that will immediately clue us in to which set we're looking at,
    like different background colors or a corner ribbon or something...

@downeym

This comment has been minimized.

Member

downeym commented Mar 12, 2018

@adammichaelwood @jbeorse I haven't dived into the code, but would a quick CSS color change for background of the sidebar be sufficient? Perhaps using the 3 colors of the current ODK logo (blue, brown, green) the various groups?

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

Perhaps using the 3 colors of the current ODK logo (blue, brown, green) the various groups?

I'm not sure exactly what you're proposing here.

a quick CSS color change for background of the sidebar be sufficient?

One of the problems with changing a color in this theme is that color definitions happen in one place in SASS, and then get scattered all over when compiled to CSS. The result is that patching a color in CSS can sometimes require a bunch of declarations and can sometimes be hard to track all of them down. (For example, sometimes a background color is defined for a section, and then re-defined for paragraphs...)

Our handful of design tweaks haven't really touched this too much, and have all been restricted to CSS.

That doesn't mean what you're suggesting isn't a bad idea. Just that it's hard to know ahead of time what will be "quick" and what will be a pain.

@downeym

This comment has been minimized.

Member

downeym commented Mar 12, 2018

So as a specific example, what about changing the value of background-color on div.wy-side-nav-search? (The color block with the logo and search box in the upper-left.) Blue for ODK1, Green for ODK2.

And maybe the grey sidebar too if the contrast looks wonky.

Hopefully that wouldn't muck with the styling for the actual documents themselves....

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

The search box isn't always visible --- that is, if you've scrolled down.

The main color on the sidebar would be good, but I don't know what, besides dark grey, would look good. Another option is to change the main section background from light gray to white. I'm worried, though, that this would subtly reinforce "OKD2 is newer and better".

Another option is a sticky nav bar along the top (https://getbootstrap.com/docs/3.3/examples/navbar-fixed-top/ for EXAMPLE, not that we would use Bootstrap). This could be (for example) blue for ODK1 and Green for ODK2, and could additionally have the breadcrumb path to the current page and a search bar.

@downeym

This comment has been minimized.

Member

downeym commented Mar 12, 2018

@jbeorse

This comment has been minimized.

Member

jbeorse commented Mar 12, 2018

Thanks for the thorough review and comments @adammichaelwood. I agree that merging sooner and working out these kinks moving forward seems most favorable.

Just so I'm clear on what needs to be done before and after the merge, my plan right now is to:

  • Update style-test.py re @ankita240796's comments
  • Add .pyc to gitignore
  • Working with @adammichaelwood and @downeym to update the styling to give a clean visual indication of whether you're looking at ODK1 or ODK2 docs.

The rest of your bullet points sounded like items for future pull requests? Is that accurate?

Regarding the styling change, I don't have strong opinions and am generally happy to go with what the groups finds most appropriate. A nav bar and a color change to the side bar seem good to me.

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

I like that DIAL sidebar ---
I'd support a patch to do something like that here.

That could be in addition to a top nav bar.
We could settle on a main color for 1 and 2 ---
(Then maybe we could eventually switch to "ODK Blue" and "ODK Green" :) )

I think probably this can all be taken care of in iterative PRs,
and we should merge this one as soon as it is fully vetted.

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

Working with @adammichaelwood and @downeym to update the styling to give a clean visual indication of whether you're looking at ODK1 or ODK2 docs.

I think this can wait for another PR.
My suspicion is that it will touch enough things to complicate matters
if we try to add it to this one.
(Not to mention divergent PRs if it takes more than a day to complete.)

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

BTW - what are the deployment URLs going to be?

@jbeorse

This comment has been minimized.

Member

jbeorse commented Mar 12, 2018

BTW - what are the URLs going to be?

Good question. Do @wbrunette or @yanokwa have opinions on this?

I think this can wait for another PR.

Sure thing. I'll get those small fixes in now.

@wbrunette

This comment has been minimized.

Member

wbrunette commented Mar 12, 2018

I agree that most of these things can be worked on after this PR in smaller PRs that will be easy to merge and have less conflicts after multiiple PRs are done.

I propose we merge this PR as a starting point with a focus on making sure everything on ODK1 documentation continues to work as expected. We then turn the deploy back on for ODK1 and take care of the suggestions from @adammichaelwood. This will allow work on ODK2 updates with smaller PRs.

With regards to the URL, it seems like something that can be decided on post merge as future PRs will need to be done. Also, the URL will likely change anyway when ODK2 is renamed and we will likely have re-direct links.

@adammichaelwood

This comment has been minimized.

Member

adammichaelwood commented Mar 12, 2018

With regards to the URL, it seems like something that can be decided on post merge as future PRs will need to be done.

Assuming the ODK1 build files all deploy correctly to their current domain... sure.

@jbeorse

This comment has been minimized.

Member

jbeorse commented Mar 12, 2018

I've added the requested changes from @ankita240796 .

@yanokwa

This comment has been minimized.

Member

yanokwa commented Mar 13, 2018

Thanks for the great work on this all!

Agreed that we should merge and start applying whatever fixes are needed. I think it should go smoothly, but to give us the best chance, I'll start that process tomorrow when most of the folks who worked on this are up and about (afternoon, Pacific Time)

@yanokwa

I think this is safe to merge, so I'll merge, send in a PR to enable builds for ODK1, and then file issues against the minor things that should probably change.

@yanokwa yanokwa merged commit 5f86dab into opendatakit:master Mar 13, 2018

4 checks passed

ci/circleci: build-odk1 Your tests passed on CircleCI!
Details
ci/circleci: build-odk2 Your tests passed on CircleCI!
Details
ci/circleci: build-pdf-odk1 Your tests passed on CircleCI!
Details
ci/circleci: build-pdf-odk2 Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment