Skip to content
This repository was archived by the owner on Jan 22, 2026. It is now read-only.

Conversation

@clottman
Copy link
Contributor

@clottman clottman commented Nov 6, 2019

Two changes here:

  1. The navigation was set at a sticky position far down in the window, so if you had the developer tools open in the browser while looking at the page, you couldn't access the bottom of the navigation section. I fixed this by making the navigation sticky to the top of the window, and only as tall as the viewport (scrolling otherwise).

Old
sidebar cut off

New
sidebar scrolls

  1. Add links in the Popover and Overlay section to point to the 'Blocks for Popover and Overlay Components' documentation.

Glitch (shared-components) added 4 commits November 5, 2019 23:31
./lib/block.js:708175/25
./lib/stories.js:708175/179
./lib/popover.js:708175/21
./lib/popover.js:708175/82
./lib/overlay.js:708175/109
@ehmorris ehmorris requested a review from a team November 7, 2019 15:09
Copy link
Contributor

@ehmorris ehmorris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I just noticed this today!

lib/stories.js Outdated
height: calc(100% - 10.5rem);
top: 1em;
height: calc(100vh - 1em);
overflow: scroll;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we use overflow: auto here instead of scroll so that the scrollbars aren't visible unless they're needed?

Copy link
Contributor

@ttseng ttseng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great! just added a few css recommendations.

lib/stories.js Outdated
position: sticky;
top: 10.5rem;
height: calc(100% - 10.5rem);
top: 1em;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest top: 5em so that the title of the page Glitch Shared Components has more prominence than the nav

image

@clottman clottman merged commit e7d804a into master Nov 7, 2019
@clottman clottman deleted the rich-red branch November 7, 2019 17:07
@clottman clottman restored the rich-red branch March 19, 2020 19:22
@keithk
Copy link
Contributor

keithk commented Nov 10, 2020

🚀 PR was released in v0.19.0 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants