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

Interact Button in Documentation #1697

Merged
merged 6 commits into from
Jul 23, 2021

Conversation

isaacgsmith
Copy link
Member

@isaacgsmith isaacgsmith commented Jul 7, 2021

Description

Adds a button in the documentation that allows you to launch binder.

Examples

See this page for an example: https://smithis7.github.io/tardis/branch/interact_badge_doc/quickstart/quickstart.html

Type of change

  • Bug fix.
  • New feature.
  • Breaking change.
  • None of the above.

Checklist

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
    • (optional) I have built the documentation on my fork following the instructions.
  • I have assigned and requested two reviewers for this pull request.

@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #1697 (4deba21) into master (2010e81) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1697   +/-   ##
=======================================
  Coverage   61.88%   61.88%           
=======================================
  Files          63       63           
  Lines        5851     5851           
=======================================
  Hits         3621     3621           
  Misses       2230     2230           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2010e81...4deba21. Read the comment docs.

docs/conf.py Outdated Show resolved Hide resolved
@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@isaacgsmith isaacgsmith marked this pull request as ready for review July 12, 2021 16:07
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Looks great - there's one thing that I missed last time.

docs/conf.py Show resolved Hide resolved
Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

Looks as expected! Thanks for changes.

@epassaro
Copy link
Member

My opinion is we should take care of the aesthetic of the documentation. Personally, I don't like the changes so much (specially the second line of text). Also, adding a block of CSS code seems overcomplicated for a task that should be easy to solve with shields.io .

Anyway, the button works as expected and that's not an easy job! Congrats. Feel free to merge this if you are satisfied with the results.

@DhruvSondhi
Copy link
Contributor

This addition seems to be good as we are trying to integrate Binder with TARDIS code notebooks. I would agree that with @epassaro on the fact that this CSS needs to be added in the nbsphinx prolog. Prologs & Epilogs allow us to add anything in the r""" & it will be appended before the main documentation (only where nbsphinx is working). Keeping that in mind, the page may become unresponsive or laggy if the load is increased via Binder via the button.
Another thing is that there should be a way to access the Binder Repository from the Main Documentation page such that the user should not be just navigating the documentation to find a Page that has this button & then click on it to access it via Binder. It should allow the user to check for all the possible Binder Pages that can be accessed.

@jaladh-singhal
Copy link
Member

My opinion is we should take care of the aesthetic of the documentation.

@epassaro We used the same white shade text on blue shade background as on top left (where tardis home is mentioned). Can you please elaborate what do you think that can be improved?

Personally, I don't like the changes so much (specially the second line of text).

@epassaro Where do you think is better to put second line of text?

Also, adding a block of CSS code seems overcomplicated for a task that should be easy to solve with shields.io

That's true but for some users it's not identifiably clickable as @smithis7 pointed out - so I have to write CSS to create an actual button.

docs/conf.py Outdated Show resolved Hide resolved
@jaladh-singhal
Copy link
Member

jaladh-singhal commented Jul 15, 2021

I would agree that with @epassaro on the fact that this CSS needs to be added in the nbsphinx prolog

@DhruvSondhi it's already added in prolog - check the code. Did you mean something else - let me know if there's better place to put it?

@isaacgsmith isaacgsmith requested a review from epassaro July 15, 2021 20:21
@epassaro
Copy link
Member

epassaro commented Jul 19, 2021

My opinion is we should take care of the aesthetic of the documentation.

@epassaro We used the same white shade text on blue shade background as on top left (where tardis home is mentioned). Can you please elaborate what do you think that can be improved?

Personally, I don't like the changes so much (specially the second line of text).

@epassaro Where do you think is better to put second line of text?

Also, adding a block of CSS code seems overcomplicated for a task that should be easy to solve with shields.io

That's true but for some users it's not identifiably clickable as @smithis7 pointed out - so I have to write CSS to create an actual button.


I found the choice of words redundant (interact-interactive, you can x2), not sure about using a semicolon before the button. Also, I'm not a fan of title case, but I'm not a native English speaker so probably it's just in my head.

You can interact with this notebook online: Launch Interactive Version

And you can download it from GitHub.

About the second line: I found it too short, not worthing a new line.

@andrewfullard
Copy link
Contributor

Are we actually happy with this? @epassaro do you want us to wait for further discussion before merging?

@jaladh-singhal
Copy link
Member

I found the choice of words redundant (interact-interactive, you can x2)

Accounted, also made clear the benefit of launching interactive version is no need of installing locally if user is interested after playing with it then they can.

not sure about using a semicolon before the button.

It's because it's an inline button - I tried without colon, that looks more odd

Also, I'm not a fan of title case, but I'm not a native English speaker so probably it's just in my head.

I can understand it may appear to loud to some users, changed to sentence style - I missed it rule 5 in the article I shared also says so.

About the second line: I found it too short, not worthing a new line.

Accounted made it bit descriptive and complimentary to 1st line. And in same line it looks too cluttered - also bear in mind that we will reduce width of main page container to 800px (#1687) in coming days (so there won't be that much white space).

@epassaro @smithis7 check this out with all changes:
image

Copy link
Member

@jaladh-singhal jaladh-singhal left a comment

Choose a reason for hiding this comment

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

@smithis7 can you make the changes in code as per screenshot I shared (they're mostly textual edits)?

isaacgsmith and others added 2 commits July 22, 2021 17:11
Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
@jaladh-singhal jaladh-singhal merged commit 8caeca3 into tardis-sn:master Jul 23, 2021
@isaacgsmith isaacgsmith deleted the interact_badge_doc branch July 26, 2021 17:21
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 1, 2021
* first draft of button

* Create button with Jaladh's code

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* Fixing indentation problem

* Fix hovering color

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* fixing github link

* deleting last line and changing title case

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021
* first draft of button

* Create button with Jaladh's code

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* Fixing indentation problem

* Fix hovering color

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* fixing github link

* deleting last line and changing title case

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
DhruvSondhi pushed a commit to DhruvSondhi/tardis that referenced this pull request Aug 9, 2021
* first draft of button

* Create button with Jaladh's code

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* Fixing indentation problem

* Fix hovering color

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* fixing github link

* deleting last line and changing title case

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
atharva-2001 pushed a commit to atharva-2001/tardis that referenced this pull request Oct 1, 2021
* first draft of button

* Create button with Jaladh's code

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* Fixing indentation problem

* Fix hovering color

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>

* fixing github link

* deleting last line and changing title case

Co-authored-by: Jaladh Singhal <jaladhsinghal@gmail.com>
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.

6 participants