www/www-new
16
3
Fork 11
Code Issues 60 Pull Requests 48 Projects Releases Wiki Activity

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
Conversation 8 Commits 5 Files Changed 1 +8 -4
a258wang commented 2021-09-18 00:45:29 -04:00
Owner
Copy Link

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
components/OrganizedContent.module.css Outdated
@ -26,3 +27,2 @@
position: sticky;
height: 100%;
margin: calc(8rem / 16) calc(32rem / 16) 0 0;
height: calc(94vh - (20rem / 16));
a3thakra commented 2021-09-19 00:29:26 -04:00
Owner
Copy Link

?

?
a258wang commented 2021-09-19 12:48:02 -04:00
Author
Owner
Copy Link
  • 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.
n3parikh commented 2021-09-19 20:13:10 -04:00
Owner
Copy Link

This works for me!

What do you mean by "finicky"?

This works for me! What do you mean by "finicky"?
a258wang commented 2021-09-19 20:22:45 -04:00
Author
Owner
Copy Link

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 😅
a3thakra commented 2021-09-20 13:56:48 -04:00
Owner
Copy Link

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 commented 2021-09-30 01:16:25 -04:00
Author
Owner
Copy Link

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
components/OrganizedContent.module.css Outdated
@ -24,2 +24,3 @@
.nav {
.navWrapper {
top: calc(20rem / 16);
bottom: calc(60rem / 16);
a3thakra commented 2021-10-01 18:50:08 -04:00
Owner
Copy Link

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
a3thakra commented 2021-10-01 18:52:33 -04:00
Owner
Copy Link

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
Clean up CSS
All checks were successful
continuous-integration/drone/push Build is passing
Details
04bc299f33
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.
Reviewers
No Reviewers
n3parikh
a3thakra
Labels
Clear labels
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: www/www-new#322
No description provided.
Delete Branch "amy-organized-content-sidebar-scroll"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?