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

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Dec 12, 2018

Fixes #4698

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Dec 12, 2018

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 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

@dojutsu-user dojutsu-user 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)

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

Can anybody confirm this?

@humitos
Copy link
Member

@humitos 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 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

@dojutsu-user dojutsu-user 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
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

@codecov
Copy link

@codecov 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 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

@dojutsu-user dojutsu-user commented Dec 12, 2018

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

@ericholscher ericholscher requested a review from Dec 26, 2018
Copy link
Member

@humitos humitos left a comment

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 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

@dojutsu-user dojutsu-user commented Jan 9, 2019

@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 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.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Jan 10, 2019

@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

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 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
3 checks passed
@dojutsu-user dojutsu-user deleted the use-sphinx-prompt branch Jan 10, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jan 10, 2019

💯 🎉 @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
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants