www
/
www-new
11
2
Fork 1
Code Issues 58 Pull Requests 28 Projects Releases Wiki Activity
Labels Milestones

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
Conversation 5 Commits 5 Files Changed 1
a258wang commented 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
continuous-integration/drone/push Build is passing
Details
d292771216 Add scrolling to Organized Content nav on smaller screens
a258wang requested review from n3parikh 1 year ago
a3thakra reviewed 1 year ago
components/OrganizedContent.module.css Outdated
position: sticky;
height: 100%;
margin: calc(8rem / 16) calc(32rem / 16) 0 0;
height: calc(94vh - (20rem / 16));
a3thakra commented 1 year ago
Collaborator

?

?
a258wang commented 1 year ago
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.
n3parikh commented 1 year ago
Owner

This works for me!

What do you mean by "finicky"?

This works for me! What do you mean by "finicky"?
a258wang commented 1 year ago
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 😅
a3thakra commented 1 year ago
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
continuous-integration/drone/push Build is passing
Details
6e141dc2c2 Refactor Organized Content nav styling
a258wang commented 1 year ago
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
components/OrganizedContent.module.css Outdated
.nav {
.navWrapper {
top: calc(20rem / 16);
bottom: calc(60rem / 16);
a3thakra commented 1 year ago
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
a3thakra commented 1 year ago
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
continuous-integration/drone/push Build is passing
Details
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
continuous-integration/drone/push Build is passing
Details
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
Details
The pull request has been merged as 92bf4752cd.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
n3parikh
a3thakra
Labels
Apply 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 a11y Backlog Blocked Bug Content Dependencies Design Feature Request Good First Issue In Progress Performance Priority - High Priority - Low Priority - Medium Untriaged
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#322
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'amy-organized-content-sidebar-scroll'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes