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

Fix last narrative scroll issue 578 #653

Merged
merged 2 commits into from
Sep 5, 2018
Merged

Fix last narrative scroll issue 578 #653

merged 2 commits into from
Sep 5, 2018

Conversation

emmahodcroft
Copy link
Member

Should hopefully fix the last remaining bit of issue 578

I get the 'tiny useless scroll bar' in IE, Edge, Firefox, and Chrome on Windows 10.

This should hopefully fix this. The scroll bar is still there in Chrome, IE, and Firefox, but it greyed out:
image
(Chrome)

However in Edge it is full-length (doesn't scroll that weird tiny bit), but is still selectable - that's why it's dark-grey below:
image

I haven't checked how this impacts long narrative paragraphs that scroll off the page, or mobile. Checking that this fixes it on Linux and doesn't impact how currently viewed in OS X would also be good!

(Also note in IE and Edge the blue dots all cluster to the left... but I don't know if this is an 'issue' - it doesn't really impact functionality.)

Copy link
Member

@jameshadfield jameshadfield left a comment

Choose a reason for hiding this comment

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

Thanks @emmahodcroft -- I can't test, but this hasn't broken anything on OS X.

re: the blue dots bunching on the left. This is a bug and shows up on iPhones, but not on my laptop so it's hard to fix. Would you mind having a go at this?

re: long narrative paragraphs that scroll off the page, this is a known bug and will be fixed in a future commit.

@@ -219,6 +220,7 @@ class App extends React.Component {
<Narrative
height={availableHeight - narrativeNavBarHeight}
width={sidebarStyles.width}
style={{overflow: "hidden"}}
Copy link
Member

Choose a reason for hiding this comment

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

I think this line is a red-herring. At least for me, it doesn't make a difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this must have been a hangover from a different idea I tried first. I'll remove it.

@@ -185,7 +185,8 @@ class App extends React.Component {
height: availableHeight,
width: sidebarWidth,
maxWidth: sidebarWidth,
overflow: "scroll",
overflowY: "scroll",
overflowX: "hidden",
Copy link
Member

Choose a reason for hiding this comment

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

The use of the scroll value is what forces the browser to display a scrollbar, even if it's unnecessary. By switching this to overflowY: "auto", we can have the browser only display a scrollbar when there is sidebar content to scroll (e.g. on a page like https://nextstrain.org/zika) and not display one when there isn't (e.g. on a narratives page).

Does that make sense / work for you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes - thanks!

@emmahodcroft
Copy link
Member Author

@jameshadfield I made a new issue for the blue dots and will have a go :)

@tsibley
Copy link
Member

tsibley commented Sep 5, 2018

@jameshadfield Will you re-test the new changes on macOS?

@jameshadfield
Copy link
Member

Certainly. Is this now working on linux and windows?

@tsibley
Copy link
Member

tsibley commented Sep 5, 2018

I believe so.

@jameshadfield
Copy link
Member

👍
closes #578

@jameshadfield jameshadfield merged commit 966854c into master Sep 5, 2018
@jameshadfield jameshadfield deleted the 578 branch September 5, 2018 18:14
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.

3 participants