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

Minor docs updates #2669

Merged
merged 6 commits into from
Jan 11, 2020
Merged

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Jan 6, 2020

This PR:

@mwcraig mwcraig added the docs label Jan 6, 2020
@mwcraig mwcraig added this to the 8.0 milestone Jan 6, 2020
@pbugnion
Copy link
Member

pbugnion commented Jan 7, 2020

This looks great! I think the clarification on the password widget is useful.

This needs to be rebased off Jason's PR (#2668) that bumps nbformat on the example notebooks to avoid build errors. For reference, I tried this here and had no issues.

@jtpio
Copy link
Member

jtpio commented Jan 7, 2020

How about also renaming the name of the conda environment to ipywidgets_docs (with a s) for consistency?

@mwcraig
Copy link
Contributor Author

mwcraig commented Jan 7, 2020

The environment is renamed; it looks like #2660 covers similar ground -- should I pull in some of those changes here? Or merge #2660 and then rebase?

@pbugnion
Copy link
Member

pbugnion commented Jan 7, 2020

My preference would be on merging this independently of #2660 to avoid coupling the two, since there are still open unresolved questions on #2660.

@mwcraig
Copy link
Contributor Author

mwcraig commented Jan 7, 2020

I'll address the maps question over there, but #2660's solution to the double-widgets problem is definitely better than this one, so we'll want to incorporate that in some way.

@@ -7,7 +7,7 @@ The Jupyter widgets packages are developed in the `https://github.com/jupyter-wi
Scope of ipywidgets
-------------------

``ipywidgets`` is a framework to provide eventful python objects that have a representation in the browser (see :doc:`examples/Widget Basics.ipynb` for more on the definition of widgets). This requires two components:
``ipywidgets`` is a framework to provide eventful python objects that have a representation in the browser (see "Simple Widget Introduction" in the :doc:`user_guide` for more on the definition of widgets). This requires two components:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simply drop the file extension to fix the link.

Sphinx document names don't include a file suffix.

http://www.sphinx-doc.org/en/master/usage/restructuredtext/roles.html#role-doc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; several of your fixes in #2660 are more elegant than what is in here...

@pbugnion
Copy link
Member

pbugnion commented Jan 9, 2020

@mwcraig I think the only thing still relevant in this PR is the password documentation?

@mwcraig
Copy link
Contributor Author

mwcraig commented Jan 9, 2020

@pbugnion -- there are a few minor things which should be more apparent in this rebase. Ordering the dependencies alphabetically is a little...odd...but makes it easier to catch duplicate package names.

@pbugnion
Copy link
Member

pbugnion commented Jan 9, 2020

@mwcraig thanks for clarifying and for pushing this through!

It's probably worth noting that the docs don't build with conda (see PR #2708 ), so I'm not sure whether it's worth fixing the requirements here?

We could merge just the password change and the broken link and hold off on the requirements fix until #2708 is merged?

@mgeier
Copy link
Contributor

mgeier commented Jan 9, 2020

Why are you adding jupyter_sphinx again (which I meticulously removed in #2660)?

And BTW, alphabetic order is great!
Unless that makes a difference for conda's dependency resolution algorithm?

@mwcraig
Copy link
Contributor Author

mwcraig commented Jan 9, 2020

Why are you adding jupyter_sphinx again (which I meticulously removed in #2660)?

Apparently alphabetical order wasn't as useful as I thought because I skimmed right over it, need better 👓. Will remove in a moment.

@mwcraig
Copy link
Contributor Author

mwcraig commented Jan 9, 2020

@pbugnion -- I don't have strong feelings about the conda fix; I was building the environment locally to test but it is honestly a hassle because you have to install the dev version of ipywidgets into this separate environment after you create it.

I'm going to push the removal of jupyter_sphinx here so that is done.

Then 2 options, you choose:

  1. Merge this as-is, rebase Switch installing documentation dependencies to pip #2708. Update dev docs instructions in Switch installing documentation dependencies to pip #2708 to drop the conda environment stuff.
  2. Do only the notebook fixes here, drop jupyter_sphinx in Switch installing documentation dependencies to pip #2708. Update dev docs instructions in Switch installing documentation dependencies to pip #2708 to drop the conda environment stuff.

I can do either of these but just need to know the preferred route. Either way we end up at correct documentation and simplified requirements.

@pbugnion
Copy link
Member

Looks great! I vote for 1 -- merging as is and rebasing 2708.

Happy to merge when you're ready!

@jasongrout
Copy link
Member

jasongrout commented Jan 11, 2020

Merged in master.

@jasongrout jasongrout merged commit 5042b8c into jupyter-widgets:master Jan 11, 2020
@jasongrout
Copy link
Member

Thanks!

@lock lock bot added the resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
docs resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation notebook output duplicated broken link in documentation Password missing from documentation
5 participants