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

Make $ unselectable in docs #4990

Merged
merged 13 commits into from
Jan 10, 2019
Merged

Make $ unselectable in docs #4990

merged 13 commits into from
Jan 10, 2019

Conversation

dojutsu-user
Copy link
Member

Fixes #4698

@dojutsu-user
Copy link
Member Author

This did fix the issue
but there's another problem
It now doesn't looks good

screenshot from 2018-12-12 15-54-15

@dojutsu-user
Copy link
Member Author

dojutsu-user commented Dec 12, 2018

Also, this PR fixes this problem
(Link)
screenshot from 2018-12-12 16-03-39
(Note that both bullets are same numbered)

Also, there's one line between those two points which is not visible in the current docs (fixed in the PR).

@dojutsu-user
Copy link
Member Author

I don't know if this is a problem with my internet connection or something.
But these images never load (Link)

screenshot from 2018-12-12 16-09-09

Can anybody confirm this?

@humitos
Copy link
Member

humitos commented Dec 12, 2018

I think that none of the issue that you are mentioning here are from your PR. I see all of them already happen in the current production online docs.

@humitos
Copy link
Member

humitos commented Dec 12, 2018

I don't know if this is a problem with my internet connection or something.
But these images never load (Link)

I do see the problem in the current docs. @davidfischer do you know what this can be happening?

@dojutsu-user
Copy link
Member Author

I think that none of the issue that you are mentioning here are from your PR. I see all of them already happen in the current production online docs.

@humitos
I didn't get it.
This problem (comment) gets fixed.
This problem (comment) gets introduced.
And this (comment) is I am asking for the confirmation (not introduced in this PR).

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

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

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #4990 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4990   +/-   ##
=======================================
  Coverage   76.76%   76.76%           
=======================================
  Files         158      158           
  Lines        9953     9953           
  Branches     1245     1245           
=======================================
  Hits         7640     7640           
  Misses       1980     1980           
  Partials      333      333

@codecov
Copy link

codecov bot commented Dec 12, 2018

Codecov Report

Merging #4990 into master will increase coverage by 0.1%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #4990     +/-   ##
=========================================
+ Coverage   76.76%   76.86%   +0.1%     
=========================================
  Files         158      158             
  Lines        9953     9987     +34     
  Branches     1245     1253      +8     
=========================================
+ Hits         7640     7677     +37     
+ Misses       1980     1977      -3     
  Partials      333      333
Impacted Files Coverage Δ
readthedocs/builds/views.py 90.74% <0%> (-5.26%) ⬇️
readthedocs/doc_builder/constants.py 86.36% <0%> (-1.14%) ⬇️
readthedocs/projects/tasks.py 70.48% <0%> (-0.72%) ⬇️
readthedocs/core/views/__init__.py 70.58% <0%> (-0.43%) ⬇️
readthedocs/projects/views/private.py 79.63% <0%> (-0.42%) ⬇️
readthedocs/doc_builder/environments.py 86.24% <0%> (-0.04%) ⬇️
readthedocs/core/models.py 74.19% <0%> (ø) ⬆️
readthedocs/builds/models.py 78.83% <0%> (ø) ⬆️
readthedocs/projects/constants.py 100% <0%> (ø) ⬆️
readthedocs/doc_builder/backends/sphinx.py 69.8% <0%> (ø) ⬆️
... and 7 more

@dojutsu-user
Copy link
Member Author

@stsewd
Thanks, I was wondering why the tests are failing.

@ericholscher ericholscher requested a review from a team December 26, 2018 20:44
Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for continue working on this PR. It looks great!

I noticed just minor changes to be made, and we are ready to merge.

docs/custom_installs/local_rtd_vm.rst Outdated Show resolved Hide resolved
docs/guides/manage-translations.rst Outdated Show resolved Hide resolved
docs/guides/manage-translations.rst Outdated Show resolved Hide resolved
@humitos
Copy link
Member

humitos commented Jan 9, 2019

This is perfect but I just realize that CSS does not look great 😞

The first one is how it looks by using :: (Sphinx default) and the second one is how it looks with .. prompt::

captura de pantalla_2019-01-09_18-57-42

I'd like to find a way to make .. prompt:: to look with the same style as the other command (or verbatim text) we have.

@dojutsu-user
Copy link
Member Author

@humitos
I didn't find any useful information about changing the css in the docs for sphinx-prompt.
My suggestion:

  1. We can add some extra css to sphinx_rtd_theme, which might not be a good idea if it clashes with some other css class.
  2. Don't use sphinx-prompt and find another way to do so.

@humitos
Copy link
Member

humitos commented Jan 10, 2019

I didn't find any useful information about changing the css in the docs for sphinx-prompt.

I also took a quick look and didn't find a way to configure it. It may worth to open an issue at sphinx-prompt issue tracker and ask for this. Maybe there is a way to tell it which classes to use.

  1. We can add some extra css to sphinx_rtd_theme, which might not be a good idea if it clashes with some other css class.

This is not something that we need at the theme level, but at project level. So, I wouldn't put it in the sphinx_rtd_theme

We may want to add an extra CSS file for our own docs project with the proper CSS classes (probably alias of what we already have).

  1. Don't use sphinx-prompt and find another way to do so.

I don't know another thing that works here.

I'm sorry that this PR is taking too long and when I was happy with the code the style of sphinx-prompt disappointed me. I'm not sure what path to follow yet, but I would start opening an issue on them and continue with the idea of adding a custom CSS.

@humitos humitos added Status: blocked Issue is blocked on another issue and removed Status: blocked Issue is blocked on another issue labels Jan 10, 2019
@dojutsu-user
Copy link
Member Author

@humitos

We may want to add an extra CSS file for our own docs project with the proper CSS classes (probably alias of what we already have).

I agree on this. And I have pushed some changes following this idea. It is working as it should be and the code-blocks also looks good.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Nice work @dojutsu-user! This looks great to me!

This is the result that I'm getting locally:

captura de pantalla_2019-01-10_11-33-26

I'd say that we are ready to merge this PR.

docs/_static/css/sphinx_prompt_css.css Outdated Show resolved Hide resolved
@humitos
Copy link
Member

humitos commented Jan 10, 2019

Thanks a lot for all of this work! I'm merging.

@humitos humitos merged commit dde80f8 into readthedocs:master Jan 10, 2019
@dojutsu-user dojutsu-user deleted the use-sphinx-prompt branch January 10, 2019 10:57
@ericholscher
Copy link
Member

💯 🎉 @dojutsu-user

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.

4 participants