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

Sphinx docs #3034

Merged
merged 175 commits into from
Mar 3, 2015
Merged

Sphinx docs #3034

merged 175 commits into from
Mar 3, 2015

Conversation

MCGallaspy
Copy link
Contributor

Just to keep track of this work. I'll be merging in changes to this throughout the week. Also, feel free to comment on the docs. The main action, so to speak, is in the kalite/sphinx-docs directory. This also incorporates @cpauya's screenshot management command PR (with some modification).

To make the documentation, run make html in the sphinx-docs directory. It might take some time to create screenshots, as it needs to fire up an instance of the server and a browser.

Changes to browser_mixins just extends one method's element selection to include css classes by an optional argument (so no backwards breaking changes) and adds in a missing import that caused an error for me.

mrpau-dev and others added 30 commits February 4, 2015 15:35
…ads list of screenshots to take from `screenshots.json` file.

Added `/data/screenshots` folder and `ghostdriver.log` to `.gitignore` file.
…he form, take screenshots between inputs.

Make re-usable the reset and delete sqlite database methods.
More efficient selenium browsing code by checking if browser's current url is already the same as the `start_url` or if the last user is the same user for the screenshot so no login is needed.
Change keys on .json for consistency and make these keys constants.
Specify field names on the `actions` array - "Explicit is better than implicit."
Needs to be refactored. Probably good to decouple from
TestCase framework. Are there other unnecessary parts that
could affect performance? Don't want to slow down sphinx
build process.
Add two command line options, --output-dir which
lets you specify an output directory for the screenshots
relative to the project root url, and --from-str so that
we can just pass a JSON string to the command (instead of
reading from a file).
Also added the ability to specify verbosity, namely
now -v 0 gives no output (except for some mysterious
TestCase stuff???).
Entered some sort of import hell...
At least it's possible to invoke the management command
by an os.system call, but it's not pretty.
And some other minor changes:
* Handle empty arg_str for _parse_nav_steps
* In _parse_nav_steps change return type of `args` keyword
in order to conform with the invocation in `run` method
* Removed _commands_handler as it was rendundant and folded
the case of multiple commands into _command_handler
In other words, there's no difference between 1 and many commands now
* Prettify _command_handler and _login_handler by moving some shared
info into instance variables set in `run`
* Added some implementation-specific helper functions to the end of
the file

Is this file getting too cluttered?
screenshot.py wants to `from selenium import` something,
so add ka-lite/python-packages to conf.py (for `make` commands)
and to the test script (so tests run without import errors).
This is dependent on the current directory structure.
The screenshots are collected in a build environment variable
and executed as a batch at a certain point in the build process
This will significantly decrease the build time.
Prevents a potential error for multiple shots if one fails
and the browser remains open.
@MCGallaspy
Copy link
Contributor Author

Are we ready to merge this in to develop? I'll do the merge conflict resolution if someone else okays it. Then we can continue to push changes to the docs as we make tweaks. I just want to get rid of this monolithic PR so that reviewing changes is easier.

@rtibbles
Copy link
Member

@aronasorman Just set off for the Philippines, but he has a long layover in Bangalore that he may be able to look at this during. I don't feel familiar enough with it to merge it.

@MCGallaspy
Copy link
Contributor Author

@rtibbles fair enough. It doesn't touch the main KA lite app too much, so in that regard it's fairly self-contained.

@aronasorman
Copy link
Collaborator

Merge conflict.

@aronasorman
Copy link
Collaborator

I'll do once more read through it, and if that's good and the conflict has been resolved we can merge this.

"start_url": "/",
"inputs": [],
"pages": [5],
"notes": "Does this annotation work?",

This comment was marked as spam.

This comment was marked as spam.

@aronasorman
Copy link
Collaborator

Some comments about the documentation. Since some of these are critical, (such as the ones about documentation blocked by bugs) let's wait for them to get fixed before merging this to develop.

@MCGallaspy
Copy link
Contributor Author

The JS linter is having issues with the minified qtip2 file I included for better screenshot tooltips. Suggestions?

@rtibbles
Copy link
Member

rtibbles commented Mar 3, 2015

@MCGallaspy Try putting it in /static-libraries instead (there are separate js and css folders in there), pretty sure the linter doesn't hit those as we have several other minned files in there.

@aronasorman
Copy link
Collaborator

Tests pass, and looks like my comments were addressed. Merging!

aronasorman added a commit that referenced this pull request Mar 3, 2015
@aronasorman aronasorman merged commit 36d63ad into learningequality:develop Mar 3, 2015
@aronasorman aronasorman removed the has PR label Mar 3, 2015
@aronasorman aronasorman deleted the sphinx-docs branch March 3, 2015 19:23
@rtibbles
Copy link
Member

rtibbles commented Mar 4, 2015

Woohoo!

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.

9 participants