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

Theme + css #1 #856

Merged
merged 21 commits into from
Mar 18, 2022
Merged

Theme + css #1 #856

merged 21 commits into from
Mar 18, 2022

Conversation

felixdittrich92
Copy link
Contributor

@felixdittrich92 felixdittrich92 changed the title Theme + css Theme + css #1 Mar 16, 2022
@fg-mindee fg-mindee self-assigned this Mar 16, 2022
@fg-mindee fg-mindee self-requested a review March 16, 2022 14:02
@fg-mindee fg-mindee added type: enhancement Improvement topic: build Related to dependencies and build ext: docs Related to docs folder labels Mar 16, 2022
@fg-mindee fg-mindee added this to the 0.6.0 milestone Mar 16, 2022
Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

The new theme is quite nice thanks!
However, I'd argue there is one thing that was dropped and that shouldn't:

  • the Google analytics ID (it's not used for ads at all, but this is the way Google Site Manager identifies the website and provides us with good SEO)

Is it not compatible with this theme?

docs/requirements.txt Outdated Show resolved Hide resolved
docs/source/conf.py Show resolved Hide resolved
@fg-mindee
Copy link
Contributor

Also, do you know if it's possible to reduce a bit the font size? (so that the title on the landing page fits by default)

@felixdittrich92
Copy link
Contributor Author

@fg-mindee
title was no title now i think its ok :)
But yes for font size changes i will check later (follow up PR)

@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #856 (c66a095) into main (2b70a94) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #856      +/-   ##
==========================================
+ Coverage   94.82%   94.84%   +0.01%     
==========================================
  Files         133      133              
  Lines        5200     5200              
==========================================
+ Hits         4931     4932       +1     
+ Misses        269      268       -1     
Flag Coverage Δ
unittests 94.84% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
doctr/transforms/functional/base.py 97.10% <0.00%> (+1.44%) ⬆️

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 2b70a94...c66a095. Read the comment docs.

Copy link
Contributor

@fg-mindee fg-mindee 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 the GA edit! I added a comment as I'm not positive this will do it

@@ -114,6 +114,25 @@
# A list of files that should not be packed into the epub file.
epub_exclude_files = ['search.html']

# Add googleanalytics id
# ref: https://github.com/sphinx-contrib/googleanalytics/blob/master/sphinxcontrib/googleanalytics.py
Copy link
Contributor

Choose a reason for hiding this comment

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

according to the ref https://github.com/sphinx-contrib/googleanalytics/blob/master/sphinxcontrib/googleanalytics.py#L30-L31

it looks like there are additional like to add 🤔
We have to make sure it's the correct way before moving on with this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fg-mindee I have updated this to a newer version.
But any idea how to test without deploy ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how to test it, but worst case scenario, we can revert this

About your last URL, what is this supposed to be? 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't have to set googleanalytics_id or googleanalytics_enabled then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fg-mindee
Yes, I would also say that we try it !
We do not need to set googleanalytics_enabled it is only used if you use it as extension but we have integrated it directly so no need to set.
googleanalytics_id is only added to app.config to have a better overview we could set it also directly instead but i would say keep it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

URL shows that your id is set correctly with its configuration so i think it will work :)
{"function":"__gct","vtp_trackingId":"G-40DVRMX8T4","vtp_sessionDuration":0,"tag_id":1}

@felixdittrich92
Copy link
Contributor Author

felixdittrich92 commented Mar 18, 2022

@fg-mindee
now headline is also fixed (responsive)
1
2

Copy link
Contributor

@fg-mindee fg-mindee left a comment

Choose a reason for hiding this comment

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

Thanks Felix, looks good to me :)

@fg-mindee fg-mindee added the topic: documentation Improvements or additions to documentation label Mar 18, 2022
@fg-mindee fg-mindee merged commit c08c0bd into mindee:main Mar 18, 2022
@felixdittrich92 felixdittrich92 deleted the theme branch March 18, 2022 15:28
@frgfm frgfm mentioned this pull request Jun 28, 2022
85 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext: docs Related to docs folder topic: build Related to dependencies and build topic: documentation Improvements or additions to documentation type: enhancement Improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants