-
Notifications
You must be signed in to change notification settings - Fork 20
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
Makes planning actions sticky #378
Makes planning actions sticky #378
Conversation
assets/css/availability-table.scss
Outdated
@@ -75,3 +75,15 @@ table.availability-table { | |||
color: white; | |||
} | |||
} | |||
|
|||
@media screen and (min-width: 1001px) { |
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.
Why are you updating the user availability table, as this PR is related to the planning page ?
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.
My bad, I put the rules in the wrong place, fix in in progress.
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.
Those rules are now removed, all the rules needed are in planning.scss.
assets/css/planning.scss
Outdated
@@ -169,3 +169,15 @@ $tableBoxSize: 40px; | |||
} | |||
} | |||
} | |||
|
|||
@media screen and (min-width: 1001px) { |
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.
Those values should be the default one, and you should only use media query for the mobile versions
assets/css/planning.scss
Outdated
@media screen and (min-width: 1001px) { | ||
.planning-actions-container { | ||
position: sticky; | ||
top: 0; |
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.
Why are you hiding all the table sticky headers ?
assets/css/planning.scss
Outdated
position: sticky; | ||
top: 0; | ||
z-index: 1; | ||
width: 100vw; |
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 must put a size to the buttons container, as this line will create very large buttons (you can have a look to the screenshot I've put on your issue)
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.
this width is not required for this PR, it's just to have the form actions container beeing not "floating" into the page but that's a cosmetic issue, the real need is to have the actions constantly accessible.
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 have a look to the screenshot I've put in the issue: the table headers are hidden, and buttons are now too large
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.
Yes I've seen the screenshot the PR is just not up to date.
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.
The table headers are fixed, Chrome doesn't apply the shift on top
value on thead
it has to be set on th
cells.
I have add a .planning-actions-container-wrapper
div to keep a background on the actions and prevent them to be "floating" in the page without removing the bootstrap sizing on action buttons.
ab52e4f
to
5e90df0
Compare
I've a new issue on Chrome: the div is now fixed on the left, but if I only display 2 or 3 days, I can scroll on the right event if the page is fully displayed. When "scrolling" on the right, the page and and the horizontal scrollbar are growing at the same time. It's weird, I can make a video if you need it to understand the problem |
I did not succeed to reproduce this bug but I've had a constraint to prevent the floating behavior to be triggered if the table width is smaller than the window, I should've done this at the beginning. |
2bda228
to
1360d58
Compare
@@ -2,8 +2,18 @@ | |||
|
|||
$tableBoxSize: 40px; | |||
|
|||
.planning-actions-container-wrapper { | |||
width: 100vw; |
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.
This line forces the page to always have an horizontal scrollbar event with only one displayed day
There is still the bug, but with bigger screen. In order to reproduce:
=> I can show you on Discord, by sharing my screen |
That's ok I can reproduce it on Chrome Windows with Browserstack, on Mac OS the scroll is handled differently. |
Hi @laurentChin , any update? |
Hi @mRoca, no update at this point I’m not satisfied with any of my fixes, they are over-complicated for the behavior I expect, so I’m gonna rewrite the initial implementation to make it more elegant. I just need to find some time to work on it. |
Closes #377