Debug navbar code to fix logo broken link #333

kakirastern · 2019-02-19T15:44:00Z

Fixes #323.

Attempts to mend broken Astropy logo banner link on the tutorial pages, in order for the image on the left hand side of the Learn Astropy navbar to show properly.

agarwalrounak · 2019-02-19T15:50:58Z

I don't think this fixes the issue. What you have done is change the src of astropy_logo_banner.png whereas the image on the left hand side of the Learn Astropy navbar is astropy_logo.png

You probably have to make changes in line 44 of navbar.html

kakirastern · 2019-02-19T16:26:22Z

Thanks @agarwalrounak! Was a bit too rash and brash, and have neglected to check everything thoroughly. Thanks for pointing it out. A second commit's been made to render everything internally consistent.

kakirastern · 2019-02-22T20:37:03Z

Hi @agarwalrounak, would you like to check if further changes needed to be made in this PR, and let me know if there is any suggestion to improve on the commits?

agarwalrounak · 2019-02-23T17:14:48Z

I don't think changing the image source will solve the issue. Have you tried to test your changes locally?

I have my exams going on and haven't got the time to set up the project on my system. Otherwise, I would have been able to help you in this regard.

kakirastern · 2019-02-23T17:31:30Z

Hi @agarwalrounak! Yup, I am in the process of testing the changes locally. But will take a while, as this is my first time. I was counting on giles to do the work for me, but now that it's gone I cannot rely on it anymore.

No worries, just wanted to see if you would like to contribute more in order to prepare for GSoC 2019 if OpenAstronomy will be selected again as a mentoring organization. All the best on your exams!

kakirastern · 2019-02-23T20:26:20Z

Cannot fix the broken logo link yet, but did debugged other issues with the code. Now the local build works perfect.

kakirastern · 2019-02-23T22:19:37Z

@MananAgarwal Turned out the sidebars including the navbar got directed to the "build/html/rst-tutorials/" directory automatically instead of the desired "build/html/" directory, so the logo path link was broken. I "tricked" the system with the path "../_static/astropy_logo.png" to restore the link to the source of the image. Anyway it's my poor man's way to fixing things for now. Also, I found other bugs in the themes/template/ code files, and have corrected them in this PR. You might want to look into it further, or not.

The logo on the upper left corner is finally showing with external linkage to http://www.astropy.org/astropy-tutorials/ now.

MananAgarwal · 2019-02-24T06:47:29Z

Hey @kakirastern, It's good to see you work so hard to solve this issue.
Even I had thought of this approach to "trick" the system, but this comes with a loss of generality. You are assuming that the "_static" folder is always in the parent directory of the current folder, which is true for the tutorials (at least in the current file structure). Your approach will break when you open pages like say, "contributing.html" or "dev.html" etc, as these pages are in the same directory as "_static" folder.

Other changes that you have made in the template code files are semantic changes. They definitely improve the consistency of the codebase. Thanks for pointing these out.

kakirastern · 2019-02-24T09:37:05Z

Hey @MananAgarwal, thanks for the reply! Sure, at least things work now as far as the upper left corner logo is concerned... Hopefully this quick fix will hold up for a while.

MananAgarwal · 2019-02-24T09:59:50Z

Maybe I didn't convey myself properly. This implementation is breaking in some pages.
@kakirastern Check "contributing.html" (link to latest giles build)

kakirastern · 2019-02-24T11:02:26Z

Yup, have worked out a functional fix...

kakirastern · 2019-02-24T12:32:58Z

I have a feeling we are on the right track, as the real bugs in the tutorials are finally showing... am working on these so as to pass Circle CI and Travis CI one-by-one.

kakirastern · 2019-02-24T12:53:44Z

Everything is okay for now. Will see if this can be merged soon. Thanks @agarwalrounak and @MananAgarwal!

adrn

Thanks for this! I have a few suggestions:

It seems like there are a bunch of commits in here that aren't directly related to the PR theme. In the future, just try to keep an eye on this: in general we like to try to keep the number of commits per PR small, and only keep changes directly related to the PR.
I'm confused about some of the changes here (the ones that seem relevant to this PR), so I've left some inline comments.

Given the complex commit history here, I think the best way forward would be to create a new branch and PR with the changes I suggest, unless you feel comfortable rebasing this one.

Thanks for your work on this!

adrn · 2019-02-26T14:19:58Z

tutorials/conf.py

@@ -96,7 +96,12 @@
 # The name of an image file (within the static path) to use as favicon of the
 # docs.  This file should be a Windows icon file (.ico) being 16x16 or 32x32
 # pixels large.
-html_favicon = 'astropy_favicon.ico'
+html_favicon = "_static/astropy_favicon.ico"


This shouldn't be changed: if you look at the comment above this block, it says "the name of an image file (within the static path)", so in this case you don't need to preface it with _static

I tried it without the _static/ path and an error was thrown. And I double-checked on the Internet and someone mentioned that it should be relative to the confdir location, and not relative to the _static/ path... Could you double check for me, maybe it has to do with the html_path_static path defined after this line? I built the docs locally and if I do what you are suggested this file cannot be found.

Strange - this does remove the WARNING but I don't understand why it seems to work even without it. In any case, keep it!

Sure, no problem!

tutorials/conf.py

adrn · 2019-02-26T14:21:12Z

tutorials/notebooks/Coordinates-Intro/Coordinates-Intro.ipynb

@@ -45,6 +45,7 @@
    "from astropy import units as u\n",
    "from astropy.coordinates import SkyCoord\n",
    "from astropy.table import Table\n",


The changes in this file seem unrelated to this PR.

I was trying to get Circle CI and Travis CI passed, since some errors related to this file was thrown after I fixed the broken logo link.

No problem, but let's fix the travis build in a separate PR, since this is a change to the tutorial content

adrn · 2019-02-26T14:21:24Z

tutorials/notebooks/coordinates/coordinates.ipynb

@@ -30,6 +30,7 @@
    "from astropy import units as u\n",
    "from astropy.coordinates import SkyCoord\n",
    "from astropy.table import Table\n",
+    "from astropy.utils import data\n",


The changes in this file also seem unrelated to this PR.

Similar to previous comment, I was trying to get Circle CI and Travis CI passed, since some errors related to this file was thrown after I fixed the broken logo link.

adrn · 2019-02-26T14:21:48Z

tutorials/themes/tutorials-theme/layout.html

@@ -27,7 +27,7 @@
 <!DOCTYPE html>
 {%- endblock %}



Is this a stylistic change? And are we sure both names are valid?

I changed navBar to nav_bar so that there will definitely be no confusion in case the system used is not case sensitive.

tutorials/themes/tutorials-theme/navbar.html

adrn · 2019-02-26T14:26:52Z

tutorials/themes/tutorials-theme/theme.conf

@@ -44,7 +44,7 @@ globaltoc_includehidden = true

 # HTML navbar class (Default: "navbar") to attach to <div> element.
 # For black navbar, do "navbar navbar-inverse"
-navbar_class = navbar
+navbar_class = navbar navbar-inverse


Can you explain this addition?

We have a dark navbar, so navbar-inverse supposedly should be added. Just in case.

kakirastern · 2019-02-26T14:42:17Z

I can rebase and squash if necessary... Please advise if any other changes need to be made.

adrn · 2019-02-26T14:59:44Z

tutorials/themes/tutorials-theme/navbar.html

 <nav class="navbar navbar-expand-lg bg-navbar">
-  <a class="navbar-brand" href="/astropy-tutorials"style="padding: 0px 5px;"><img src="{{ pathto('_static/' + 'astropy_logo.png') }}" style="height:38px;"></a>
+  <a class="navbar-brand" href="http://www.astropy.org/astropy-tutorials/" style="padding: 0px 5px;">


We don't want to assume the domain here, so change to href="/astropy-tutorials"

kakirastern · 2019-02-26T15:34:54Z

@adrn Commits rebased and squashed into one. Okay to merge now. Please double check if necessary.

kakirastern · 2019-02-26T15:44:20Z

@adrn I will open another PR to address the "bugs" showing up on Travis...

adrn · 2019-02-26T15:46:36Z

tutorials/conf.py

-html_favicon = 'astropy_favicon.ico'
+html_favicon = "_static/astropy_favicon.ico"
+
+# (Optional) Logo. Should be small enough to fit the navbar (ideally 24x24).


One more thing: please remove these commented-out lines, as we decided they are unnecessary. Thanks!

Yup, removed...

Change src relative location to fix logo broken link Update astropy_logo.png relative location to show logo image Change logo and banner paths to fix broken links Fixed missing space in L44 of navbar.html Change spacing in navbar code block Comment out button class in navbar code block to test Change bootstrap syntax in navbar code block Get rid of size specification to test navbar code block Fix the img src path for templating Change href to "#" to test navbar code block Add size specifications and empty alt to test navbar Change width and height specifications to test navbar code Change logo size to fit navbar Use "style" to specify height to test navbar code block Add html_logo assignment to theme.conf file Remove quotation marks for logo file name in theme.conf Make further changes to theme.conf file to test theme code Remove quotation marks around nav for source_link_position Change navBar to navbar to test theme code Revert changes from navbar to nav_bar for macro to test Add html_logo = "astropy_logo.png" to conf.py Remove logo portion that has been misplaced Change True back to true in theme.conf Revert to original syntax used with "style" Add missing space in code to test Change img src formatting to test navbar code Comment out astropy html favicon and logo files in conf.py Restore favicon definition statement in conf.py Clean up conf.py file Comment out button class in navbar code block to test Comment out nav-brand class to test navbar code block Rewrite navbar code snippets in navbar.html to test Change navbar code snippets back to test navbar.html Clean up navbar.html Polishing up on navbar.html to test code Clean up navbar.html further by getting rid of spaces Change width and height properties to fit logo in navbar Change html_favicon location to try and pick it up Change navbar properties to try to show logo Get rid of redundant "navbar" term in navbar.html Revert back to previous navbar.html settings Change height of logo to unhide image Change relative path of img src to try and pick up logo Change CSS attributes in navbar.html to test code Continue changing CSS attributes to test navbar code Uncomment html_logo path in conf.py file Correct logo path redirect to rst-tutorials behavior Change width of logo by implicit ratio-aspect lock Correct logo link redirection behavior To [http://www.astropy.org/astropy-tutorials/](http://www.astropy.org/astropy-tutorials/). Using HTML onerror attribute to correct remaining broken error link Fix syntax error in navbar.html file Add missing space and forward slash in L44 of navbar.html Change syntax to try and debug navbar.html Update coordinates.ipynb to debug NameResolveError NameResolveError: All Sesame queries failed. Unable to retrieve coordinates. Update Coordinates-Intro to debug NameResolveError Change data.conf.remote_timeout location and value in coordinates.ipynb Further update Coordinates-Intro.ipynb to debug Change name used in Sesame query to debug Revise syntax of img src for logo Remove html_logo from conf.py Change href for navbar-brand from html to relative path Revert Coordinates-Intro changes Get rid of redundant spaces in L93 Revert changes made to coordinates.ipynb tutorial Undo explicit print statement Remove unnecessary comments regarding html_logo

adrn · 2019-02-26T16:02:15Z

Thanks @kakirastern!

kakirastern · 2019-02-26T16:03:05Z

You are welcome, @adrn!

kakirastern · 2019-02-26T16:17:48Z

Solution not working completely. Have opened PR #339 to address that.

kakirastern · 2019-02-26T16:19:08Z

Sorry, my bad, it works now... Was definitely too rash and brash.

kakirastern changed the title ~~Change src relative location to fix logo broken link~~ Debug navbar code to fix logo broken link Feb 23, 2019

adrn suggested changes Feb 26, 2019

View reviewed changes

adrn reviewed Feb 26, 2019

View reviewed changes

adrn approved these changes Feb 26, 2019

View reviewed changes

adrn reviewed Feb 26, 2019

View reviewed changes

adrn merged commit 98a5b63 into astropy:master Feb 26, 2019

kakirastern deleted the fix-broken-logo branch February 26, 2019 16:02

Debug navbar code to fix logo broken link #333

Debug navbar code to fix logo broken link #333

Conversation

kakirastern commented Feb 19, 2019 • edited

agarwalrounak commented Feb 19, 2019

kakirastern commented Feb 19, 2019

kakirastern commented Feb 22, 2019

agarwalrounak commented Feb 23, 2019

kakirastern commented Feb 23, 2019

kakirastern commented Feb 23, 2019

kakirastern commented Feb 23, 2019

MananAgarwal commented Feb 24, 2019

kakirastern commented Feb 24, 2019

MananAgarwal commented Feb 24, 2019 • edited

kakirastern commented Feb 24, 2019

kakirastern commented Feb 24, 2019

kakirastern commented Feb 24, 2019

adrn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kakirastern commented Feb 26, 2019

Choose a reason for hiding this comment

kakirastern commented Feb 26, 2019

kakirastern commented Feb 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrn commented Feb 26, 2019

kakirastern commented Feb 26, 2019

kakirastern commented Feb 26, 2019

kakirastern commented Feb 26, 2019

kakirastern commented Feb 19, 2019 •

edited

MananAgarwal commented Feb 24, 2019 •

edited