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.

Sign up for GitHub

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
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed92ab7
Pin important wiki notices to the top of the page
cl8n Jun 14, 2022
638fb90
Factor out offset sticky thing to function
cl8n Jun 14, 2022
60afcba
Adjust scroll-padding-top if there are pinned notices
cl8n Jun 14, 2022
83f3a03
Fix switching between desktop and mobile and reset on page change
cl8n Jun 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions resources/assets/less/bem/page-extra-tabs.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,6 @@
// See the LICENCE file in the repository root for full licence text.

.page-extra-tabs {
position: sticky;
--navbar-height: @navbar-height;
// 1px overlap with navbar to prevent possible gap in some cases.
// Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=1261852
top: calc(var(--navbar-height) - 1px);
.sticky-below-navbar();
z-index: @z-index--page-extra-tabs;

@media @desktop {
--navbar-height: @nav2-height--pinned;
}
}
5 changes: 5 additions & 0 deletions resources/assets/less/bem/wiki-notice.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

.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?

}
}
12 changes: 12 additions & 0 deletions resources/assets/less/functions.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,3 +322,15 @@
/* autoprefixer: ignore next */
-webkit-box-orient: vertical;
}

.sticky-below-navbar() {
position: sticky;
--navbar-height: @navbar-height;
// 1px overlap with navbar to prevent possible gap in some cases.
// Reference: https://bugs.chromium.org/p/chromium/issues/detail?id=1261852
top: calc(var(--navbar-height) - 1px);

@media @desktop {
--navbar-height: @nav2-height--pinned;
}
}
5 changes: 3 additions & 2 deletions resources/assets/less/layout.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,11 @@ html, body {
}

html {
scroll-padding-top: @nav2-height--pinned;
--scroll-padding: 0;
scroll-padding-top: calc(var(--scroll-padding) + @nav2-height--pinned);

@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

}
}

Expand Down
16 changes: 16 additions & 0 deletions resources/assets/lib/core/wiki/pinned-notices-scroll-padding.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the GNU Affero General Public License v3.0.
// See the LICENCE file in the repository root for full licence text.

export default class PinnedNoticesScrollPadding {
constructor() {
$(document).on('turbolinks:load', this.adjustScrollPadding);
}

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

: 0;
document.documentElement.style.setProperty('--scroll-padding', `${scrollPadding}px`);
};
}
2 changes: 2 additions & 0 deletions resources/assets/lib/osu-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import UserLoginObserver from 'core/user/user-login-observer';
import UserModel from 'core/user/user-model';
import UserPreferences from 'core/user/user-preferences';
import UserVerification from 'core/user/user-verification';
import PinnedNoticesScrollPadding from 'core/wiki/pinned-notices-scroll-padding';
import ReferenceLinkTooltip from 'core/wiki/reference-link-tooltip';
import WindowFocusObserver from 'core/window-focus-observer';
import WindowSize from 'core/window-size';
Expand Down Expand Up @@ -50,6 +51,7 @@ export default class OsuCore {
notificationsWorker: NotificationsWorker;
readonly osuAudio: OsuAudio;
readonly osuLayzr = new OsuLayzr();
readonly pinnedNoticesScrollPadding = new PinnedNoticesScrollPadding();
readonly reactTurbolinks: ReactTurbolinks;
readonly referenceLinkTooltip = new ReferenceLinkTooltip();
readonly scorePins = new ScorePins();
Expand Down
30 changes: 16 additions & 14 deletions resources/views/wiki/_notice.blade.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,23 @@
</div>
@endif

@if ($page->isLegalTranslation())
<div class="wiki-notice wiki-notice--important">
{!! osu_trans('wiki.show.translation.legal', [
'default' => '<a href="'.e(wiki_url($page->path, config('app.fallback_locale'))).'">'.e(osu_trans('wiki.show.translation.default')).'</a>',
]) !!}
</div>
@endif
<div class="wiki-notice__pinned-container js-wiki-pinned-notices">
@if ($page->isLegalTranslation())
<div class="wiki-notice wiki-notice--important">
{!! osu_trans('wiki.show.translation.legal', [
'default' => '<a href="'.e(wiki_url($page->path, config('app.fallback_locale'))).'">'.e(osu_trans('wiki.show.translation.default')).'</a>',
]) !!}
</div>
@endif

@if ($page->isOutdatedTranslation())
<div class="wiki-notice">
{!! osu_trans('wiki.show.translation.outdated', [
'default' => '<a href="'.e(wiki_url($page->path, config('app.fallback_locale'))).'">'.e(osu_trans('wiki.show.translation.default')).'</a>',
]) !!}
</div>
@endif
@if ($page->isOutdatedTranslation())
<div class="wiki-notice">
{!! osu_trans('wiki.show.translation.outdated', [
'default' => '<a href="'.e(wiki_url($page->path, config('app.fallback_locale'))).'">'.e(osu_trans('wiki.show.translation.default')).'</a>',
]) !!}
</div>
@endif
</div>

@if ($page->isOutdated())
<div class="wiki-notice">
Expand Down