Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

Fix #81 - Vertical scrollbar overlays header #138

Merged
merged 7 commits into from
Jun 22, 2015

Conversation

mikestead
Copy link
Contributor

#81

I'm not too sure if this is something you think needs fixing but it was causing us a little grief by obscuring the sort arrow in the right hand columns header.

Feels a little better keeping the scrollbar inside the body of the table to my eye. Let me know what you think.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@pieterv
Copy link
Contributor

pieterv commented May 21, 2015

Hey @mikestead Thanks for the PR!

@jbonta @wlis What do you think about changing the scroll bar to not overlap the header?

…into fix/yscrollbar-offset-top

# Conflicts:
#	src/FixedDataTable.react.js
@jbonta
Copy link
Contributor

jbonta commented May 21, 2015

yeah, I think this makes sense. Does the header's shadow still cover the scrollbar? Do you have a screenshot? cc @wlis

@mikestead
Copy link
Contributor Author

@jbonta good point, the scroll bar track is sitting on top of the header shadow which looks off.

The Scrollbar z-index defaults to 99 in the component, but it can be passed a different z-index prop to use. The shadow z-index is defined in the css file. I can bump that up to 100 (see before / after images below) but not sure that's a great fix.

screen shot 2015-05-21 at 2 31 39 pm

screen shot 2015-05-21 at 2 32 04 pm

@ehzhang ehzhang merged commit aa52300 into facebookarchive:master Jun 22, 2015
@ehzhang
Copy link
Contributor

ehzhang commented Jun 22, 2015

Thanks for the PR! It's been merged in, with the adjustment to the z-index (100).

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

Successfully merging this pull request may close these issues.

5 participants