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

Merged
a258wang merged 5 commits from amy-organized-content-sidebar-scroll into main 1 year ago
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 added 1 commit 1 year ago
d292771216 Add scrolling to Organized Content nav on smaller screens
a258wang requested review from n3parikh 1 year ago
a3thakra reviewed 1 year ago
position: sticky;
height: 100%;
margin: calc(8rem / 16) calc(32rem / 16) 0 0;
height: calc(94vh - (20rem / 16));
Collaborator

?

?
Poster
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"?
Poster
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 😅
Collaborator

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.
a258wang added 1 commit 1 year ago
6e141dc2c2 Refactor Organized Content nav styling
Poster
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 1 year ago
a3thakra reviewed 1 year ago
.nav {
.navWrapper {
top: calc(20rem / 16);
bottom: calc(60rem / 16);
Collaborator

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
Collaborator

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 added 2 commits 1 year ago
b36210f77a Reorganize CSS properties
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 1 year ago
a258wang force-pushed amy-organized-content-sidebar-scroll from b36210f77a to 6df2f3fdd9 1 year ago
a258wang added 1 commit 1 year ago
04bc299f33 Clean up CSS
a3thakra approved these changes 1 year ago
a258wang merged commit 92bf4752cd into main 1 year ago

Reviewers

n3parikh was requested for review 1 year ago
a3thakra approved these changes 1 year ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 92bf4752cd.
Sign in to join this conversation.
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#322
Loading…
There is no content yet.