-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
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
[ListSubheader] Use sticky list by default #8194
Conversation
I've verified that it works on Chrome, Firefox and Safari (latest versions). Didnt check on IE or mobile yet. 1 thing that i've discovered while working with this is that if you have an |
I think that it can be good enough as long as the UI isn't broken on the browsers that don't support it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, we can move forward after fixing those comments :)
function PinnedList(props) { | ||
const classes = props.classes; | ||
|
||
const sections = Array.from(Array(SECTIONS).keys()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array.from
is not supported by IE11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have been using [1, 2, 3, ..
on the other demos to dodge the issue.
width: '100%', | ||
maxWidth: 360, | ||
background: theme.palette.background.paper, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blank line
docs/src/pages/demos/lists/lists.md
Outdated
@@ -26,6 +26,10 @@ Lists are best suited for similar data types. | |||
|
|||
{{demo='pages/demos/lists/NestedList.js'}} | |||
|
|||
### Pinned Subheader List | |||
|
|||
{{demo='pages/demos/lists/PinnedSubheaderList.js'}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add some remark around the browser support here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also say that browser that doesn't support it will render as before, the UI won't be broken.
Looking at the spec it says "Upon scrolling, subheaders remain pinned to the top of the screen until pushed off screen by the next subheader.", so I'm wondering, should this be the default behavior? |
👍 |
I'm continuing this PR :) |
18ff893
to
9eb5cb9
Compare
9eb5cb9
to
5f0b4b1
Compare
@slavab89 Thanks! |
Thanks @oliviertassinari 😄 I was on vacation so you beat me to it :) |
Fixes #7413
Please let me know what you think or if i should change anything 😄
BTW, i was able to only run tests.
flow
failed on some package innode_modules
(not sure why it ran it) andeslint
just hangs indefinitelyp.s Sticky is supported in all recent version browsers but not in all
https://caniuse.com/#search=sticky
Is it good enough?