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

Create top-level nav item for docs #4853

Merged
merged 4 commits into from
Feb 21, 2016

Conversation

MCGallaspy
Copy link
Contributor

Fixes #4375 by adding a top-level item to the menu for docs if they are detected. Appears regardless of whether the user is logged in or not. Now also links directly to the user manual section, and I changed the display name accordingly. @radinamatic your opinion on this?

Screenshot:
screenshot

@radinamatic
Copy link
Member

More immediate access to the content relevant for the user - I'd say it's a winner, @MCGallaspy!

@MCGallaspy
Copy link
Contributor Author

This failure must be unrelated to the changes:

  Scenario: Content is available        # kalite/distributed/features/learn_content.feature:10
    Given I open some available content # kalite/distributed/features/steps/learn_content.py:14
    Given I open some available content # kalite/distributed/features/steps/learn_content.py:14 2.374s
    Then I should see no alert          # kalite/distributed/features/steps/learn_content.py:31
    Then I should see no alert          # kalite/distributed/features/steps/learn_content.py:31 6.401s
      Traceback (most recent call last):
        File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/behave/model.py", line 1173, in run
          match.run(runner.context)
        File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/behave/model.py", line 1589, in run
          self.func(context, *args, **kwargs)
        File "/home/ubuntu/ka-lite/kalite/distributed/features/steps/learn_content.py", line 35, in impl
          assert_no_element_by_css_selector(context, ".alert")
        File "/home/ubuntu/ka-lite/kalite/testing/behave_helpers.py", line 95, in assert_no_element_by_css_selector
          _assert_no_element_by(context, By.CSS_SELECTOR, css_value, wait_time)
        File "/home/ubuntu/ka-lite/kalite/testing/behave_helpers.py", line 85, in _assert_no_element_by
          EC.presence_of_element_located((by, value))
        File "/home/ubuntu/virtualenvs/venv-2.7.9/lib/python2.7/site-packages/selenium/webdriver/support/wait.py", line 91, in until_not
          raise TimeoutException(message)
      TimeoutException: Message:

Link to your job: https://saucelabs.com/jobs/f9090b82c09f489cb5e91413998dd57e

@MCGallaspy
Copy link
Contributor Author

From another thread (@benjaoming):

If you can add a Help tab and only show it to logged-in users who aren't learners, and as @radinamatic mentions deeplink to the User Manual, that'd be sufficient IMO?

Good point, will address.

@MCGallaspy
Copy link
Contributor Author

However, I'm wondering if we should also show the link to non-logged-in users?

@MCGallaspy
Copy link
Contributor Author

I went ahead and made the suggested changes, @benjaoming. The link is not shown to non-logged-in users.

@MCGallaspy
Copy link
Contributor Author

Ah, test likely fails because documentation does not exist.

@@ -19,6 +19,7 @@ dependencies:
test:
override:
- make assets
- make docs

This comment was marked as spam.

@benjaoming
Copy link
Contributor

Yeah, this is super nice! Makes the docs extremely visible to the users that the docs work really well for in the current circumstances.

As for non-logged in users... hm. Good question, but I would skip it because most users who aren't logged in a Learners in the bigger picture. But there could be a link in the first-run "create admin account" modal.

@MCGallaspy MCGallaspy force-pushed the 4375-add-docs-nav-item branch from 698e6ca to 0bd6835 Compare February 18, 2016 18:16
@MCGallaspy MCGallaspy force-pushed the 4375-add-docs-nav-item branch from 0bd6835 to 61928c1 Compare February 19, 2016 21:56
@benjaoming
Copy link
Contributor

Looks great! Sorry if I was slow at merging it -- Github doesn't create direct notifications of subscribers when new commits arrive in a PR, so just post a whatever comment about the updates to makes sure subscribers are pinged.

benjaoming added a commit that referenced this pull request Feb 21, 2016
@benjaoming benjaoming merged commit 6488c7e into learningequality:0.16.x Feb 21, 2016
@benjaoming benjaoming removed the has PR label Feb 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants