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

A couple of small weird things #12

Open
smoh opened this issue Jul 10, 2019 · 13 comments
Open

A couple of small weird things #12

smoh opened this issue Jul 10, 2019 · 13 comments
Labels

Comments

@smoh
Copy link

smoh commented Jul 10, 2019

From looking at e.g., http://docs.astropy.org/en/stable/units/index.html

I don't think these issues are particular to a web browser but I am using Firefox 67.0.4 on Mac.

  • Why is there always leftover horizontal scroll that moves the entire web page? Even when the horizontal display width >1800px.
  • The sidebar collapse indicator span element << incidentally does not move with scroll so you see them off the sidebar divider:

image

Just some things I've noticed.

@astrofrog astrofrog added the bug label Jul 10, 2019
@astrofrog
Copy link
Member

@smoh - thanks for reporting these issues! I can confirm both of them. Do you have any ideas for how to fix these?

@smoh
Copy link
Author

smoh commented Jul 10, 2019

Unfortunately, I only have vague ideas of what to google for possible fixes; I'll try later today.

@smoh
Copy link
Author

smoh commented Jul 10, 2019

So I found out this is because of

div.topbar {
    ...
    padding: 0px 10px;
    width: 100%;
    ....
}

The padding was always there to pad the logo but width:100% was introduced in
57d5014

Because width does not include padding, setting it to 100% makes it to have width=(body width) + 20px making it always go over the body=window width.

If this is fixed, the second issue goes away. Not sure how to proceed further.

[1] https://stackoverflow.com/questions/4698054/css-does-the-width-include-the-padding

@saimn
Copy link
Contributor

saimn commented Jul 10, 2019

On solution is to use box-sizing: border-box; (https://developer.mozilla.org/fr/docs/Web/CSS/box-sizing) which is I think compatible with all modern browsers.

@smoh
Copy link
Author

smoh commented Jul 10, 2019

Yes, looks like it as long as its compatible!

@smoh
Copy link
Author

smoh commented Jul 11, 2019

cc @kakirastern who put in width:100% to see if this is fine

@kakirastern
Copy link
Contributor

Hi @smoh

I am sure if that has to do with my setting the width to 100% for the topbar only, since the two divisions are unrelated... But there should some be no zero-width collapse bar there for control, otherwise it would lose its purpose there. (If it disappears after collapsing you then cannot un-collapse the pane.)

This is what I see on Google Chrome BTW, which from what I can tell is conforming to what has been intended:

Screenshot 2019-07-12 01 52 18

Screenshot 2019-07-12 01 52 29

@kakirastern
Copy link
Contributor

The topbar we have for the AstroPy website is like the bootstrap navigation bar or navbar. In fact, I am not even sure if the changes in the PR(s) I have made has been effected yet, since is so there should be more menu items on the topbar.

@smoh
Copy link
Author

smoh commented Jul 11, 2019

The issue has nothing to do with there being a divider for collapsing siddbar or would result in anything different from the screenshots you attached. It's that because topbar's actual width (which is width + padding) goes over the body you end up with a residual horizontal scroll which results in the entire page shifting horizontally unnecessarily. I checked that the problem persists in Chrome v75.0.3770.100.

I'm pretty sure the topbar width option put in is in effect because I can see it in the latest online documentation and the commit I referenced is the only one that changed div.topbar's width. Also in astropy readthedocs build log astropy-sphinxs-theme v1.1.

I was just asking if the width:100% is necessary for some purpose, the solution suggested to put in
box-sizing: border-box; additionally would be fine with that as well.

Anyway, it looks like this can be resolved easily by more than one ways and I'll leave the maintainers to it.

@kakirastern
Copy link
Contributor

kakirastern commented Jul 11, 2019

@smoh

Yeah, now after your clarification I see what your concern is. Functionally everything is okay now except aesthetically it could be kind of irksome for some.

I can try opening a PR to follow it up and help fix things, to see if the issue is fixable and if it is indeed due to my fixing the topbar width to 100%. I do not recall whether things were the same before I introduced the change, but I do remember I needed that 100% width there for things to work so it could be somewhat important.

@kakirastern
Copy link
Contributor

kakirastern commented Jul 11, 2019

Actually there are some other pending PR's in the AstroPy and this repo that is related to the commit 57d5014. I have tried to add the suggested change in this issue in one of the pending PR's through this
ee41893 just a few minutes ago. It will depend on the repo maintainer(s) whether my PR(s) will get approved or not.

@kakirastern
Copy link
Contributor

So I did a little bit digging further and found an affiliated package that still uses the old theme at Spectral-Cube, and can confirm that the extra padding without also adding box-sizing: border-box; did make the scroll sign kind of detached from the scroll bar if one "drags" the page sideways...

@kakirastern
Copy link
Contributor

Tested the solution by adding box-sizing: border-box; CSS property to div.topbar on both Google Chrome and Mozilla Firefox today. It works! Hopefully #9 or a new PR committing the same change can be merged soon to correct for the behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants