Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pin important wiki notices to the top of the page #9021

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cl8n
Copy link
Member

@cl8n cl8n commented Jun 14, 2022

so ppl will still see them when they open a wiki page from a section link, or just scroll past the notices at the beginning

looks a little strange when other not-pinned notices scroll under it, but I'm not sure if anything should be done about that

@nanaya
Copy link
Collaborator

nanaya commented Jun 14, 2022

This covers section title (and some part of the content) when using table of content or section link...

@cl8n
Copy link
Member Author

cl8n commented Jun 14, 2022

that's true... im not sure how to fix, would the best way be to get the height of this container and add it to scroll-padding/scroll-margin? or is there some way i can make this work without js 🤔


&__pinned-container {
.sticky-below-navbar();
background-color: hsl(var(--hsl-b5));
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this do anything?


@media @mobile {
scroll-padding-top: @navbar-height;
scroll-padding-top: calc(var(--scroll-padding) + @navbar-height);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use variable instead so the value using calc only need to be defined once

private adjustScrollPadding = () => {
const pinnedNotices = document.querySelector('.js-wiki-pinned-notices');
const scrollPadding = pinnedNotices instanceof HTMLElement
? pinnedNotices.getBoundingClientRect().height + 10
Copy link
Collaborator

Choose a reason for hiding this comment

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

this may be wrong if the window is resized

@@ -25,4 +25,9 @@
margin: 0;
}
}

&__pinned-container {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel in some cases this may end up annoying people with this being permanently visible and reducing actual content area when reading the page (especially on mobile)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah... talked about it with TicClick, who originally suggested idea of this PR, and we both aren't sure how to make it better. I was thinking it could be good to add a dismiss option that puts it back into its normal position, but I don't know if that's actually less "annoying", might feel like a cookie popup where ppl instantly click+close by habit

Copy link
Member

Choose a reason for hiding this comment

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

i'm not sure if this should be necessary at all... maybe the least intrusive solution could be a dismissable cookie popup only for when arriving from a section link and the original notice isn't visible (and it auto-dismisses if you scroll up to the notice)

Copy link
Contributor

Choose a reason for hiding this comment

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

given how limited screen space on mobile devices is, it makes sense to only show this on desktop.
another thought I had is that there's already an ever-floating table of contents on the left, which could be reused for just a small chip saying outdated [?] or something

@cl8n cl8n marked this pull request as draft November 7, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants