Add scrolling to Organized Content sidebar on smaller desktop screens #322

Merged
a258wang merged 5 commits from amy-organized-content-sidebar-scroll into main 2021-10-04 18:57:12 -04:00
Owner

Closes #314

Personally I think it feels a little finicky, but please let me know what y'all think!

Closes #314 Personally I think it feels a little finicky, but please let me know what y'all think!
a258wang requested review from n3parikh 2021-09-18 00:45:43 -04:00
a3thakra reviewed 2021-09-19 00:29:26 -04:00
@ -26,3 +27,2 @@
position: sticky;
height: 100%;
margin: calc(8rem / 16) calc(32rem / 16) 0 0;
height: calc(94vh - (20rem / 16));
Owner

?

?
Author
Owner
  • Due to the vertical margins on the sidebar, 100vh meant that it would still be a little bit cut off, so that's where the 94vh comes from.
  • With only height: 94vh, I noticed that the sidebar would still get a little bit clipped when the screen was small, so I added the - (20rem / 16) to mitigate that. I chose 20 rem / 16 to match the bottom margin of the sidebar.
  • I recognize that when the screen is small enough, the sidebar has zero/negative height... but I don't think anyone's going to be perusing our website with that small of a screen.
  • Looking back at this, I wonder if changing the margins to padding and setting box-sizing: border-box or something would allow us to simplify the height? EDIT: Since padding appears to be included in determining the scrollable, this doesn't really work.
- Due to the vertical margins on the sidebar, `100vh` meant that it would still be a little bit cut off, so that's where the `94vh` comes from. - With only `height: 94vh`, I noticed that the sidebar would still get a little bit clipped when the screen was small, so I added the `- (20rem / 16)` to mitigate that. I chose `20 rem / 16` to match the bottom margin of the sidebar. - I recognize that when the screen is small enough, the sidebar has zero/negative height... but I don't think anyone's going to be perusing our website with that small of a screen. - ~~Looking back at this, I wonder if changing the margins to padding and setting `box-sizing: border-box` or something would allow us to simplify the `height`?~~ EDIT: Since padding appears to be included in determining the scrollable, this doesn't really work.
Owner

This works for me!

What do you mean by "finicky"?

This works for me! What do you mean by "finicky"?
Author
Owner

This works for me!

What do you mean by "finicky"?

I'm glad to hear that it works! I'm just not sure if sidebar looks good on all different sizes of screens, plus the code is a bit messy 😅

> This works for me! > > What do you mean by "finicky"? I'm glad to hear that it works! I'm just not sure if sidebar looks good on *all* different sizes of screens, plus the code is a bit messy 😅
Owner

This works, but I think we should create a div inside the sticky div, and make that overflow-able. That way we get to keep things clean.

This works, but I think we should create a div inside the sticky div, and make that overflow-able. That way we get to keep things clean.
Author
Owner

This works, but I think we should create a div inside the sticky div, and make that overflow-able. That way we get to keep things clean.

Hmmm, it seems like position: sticky and overflow: auto need to be set on the same element in order for everything to work properly. I've pushed a new commit where I put these attributes on a surrounding div instead of directly on the nav element - please let me know what y'all think!

(Also I realized that in my haste, I completely messed up the mobile styling, so regardless of whether or not we want to keep the changes from commit 6e141dc2c2, I'll probably have to make more changes, oops.)

> This works, but I think we should create a div inside the sticky div, and make that overflow-able. That way we get to keep things clean. Hmmm, it seems like `position: sticky` and `overflow: auto` need to be set on the same element in order for everything to work properly. I've pushed a new commit where I put these attributes on a surrounding div instead of directly on the nav element - please let me know what y'all think! (Also I realized that in my haste, I completely messed up the mobile styling, so regardless of whether or not we want to keep the changes from commit 6e141dc2c2, I'll probably have to make more changes, oops.)
a258wang changed title from Add scrolling to Organized Content sidebar on smaller desktop screens to WIP: Add scrolling to Organized Content sidebar on smaller desktop screens 2021-09-30 01:16:35 -04:00
a3thakra reviewed 2021-10-01 18:50:08 -04:00
@ -24,2 +24,3 @@
.nav {
.navWrapper {
top: calc(20rem / 16);
bottom: calc(60rem / 16);
Owner

I don't think this is doing anything. Is it?

I don't think this is doing anything. Is it?
a3thakra marked this conversation as resolved
Owner

okay i think there's a very marginal improvement in terms of code quality between the new version and the old. So I'll leave it to you to decide what you wanna choose.

okay i think there's a very marginal improvement in terms of code quality between the new version and the old. So I'll leave it to you to decide what you wanna choose.
a258wang changed title from WIP: Add scrolling to Organized Content sidebar on smaller desktop screens to Add scrolling to Organized Content sidebar on smaller desktop screens 2021-10-04 00:42:24 -04:00
a258wang force-pushed amy-organized-content-sidebar-scroll from b36210f77a to 6df2f3fdd9 2021-10-04 00:43:39 -04:00 Compare
a258wang added 1 commit 2021-10-04 18:16:21 -04:00
continuous-integration/drone/push Build is passing Details
04bc299f33
Clean up CSS
a3thakra approved these changes 2021-10-04 18:17:57 -04:00
a258wang merged commit 92bf4752cd into main 2021-10-04 18:57:12 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#322
No description provided.