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

feat(ui): bottom nav #2164

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions src/inline-script/inline-script.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const {
currentInstance,
instanceThemes,
disableCustomScrollbars,
bottomNav,
enableGrayscale,
pushSubscription,
loggedInInstancesInOrder,
Expand Down Expand Up @@ -53,6 +54,8 @@ if (disableCustomScrollbars) {
.setAttribute('media', 'only x') // disables the style
}

document.body.classList.toggle('bottom-nav', bottomNav);

if (centerNav) {
document.getElementById('theCenterNavStyle')
.setAttribute('media', 'all') // enables the style
Expand Down
1 change: 1 addition & 0 deletions src/intl/en-US.js
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ export default {
theme: 'Theme',
themeForInstance: 'Theme for {instance}',
disableCustomScrollbars: 'Disable custom scrollbars',
bottomNav: 'Place the navigation bar at the bottom of the screen',
centerNav: 'Center the navigation header',
preferences: 'Preferences',
hotkeySettings: 'Hotkey settings',
Expand Down
7 changes: 6 additions & 1 deletion src/routes/_components/Nav.html
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<nav id="main-nav" class="main-nav">
<nav id="main-nav" class="main-nav{bottomNav ? ' bottom-nav' : ''}">
<ul class="main-nav-ul">
{#each $navPages as navPage (navPage.href)}
<li class="main-nav-li">
Expand Down Expand Up @@ -30,6 +30,11 @@
contain: content; /* see https://www.w3.org/TR/2018/CR-css-contain-1-20181108/#valdef-contain-content */
}

.main-nav.bottom-nav {
top: calc(100vh - 52px);
bottom: 0;
}

.main-nav-ul {
margin: 0;
padding: 0;
Expand Down
8 changes: 7 additions & 1 deletion src/routes/_layout.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<Nav {page} />

<div class="main-content">
<div class="main-content {bottomNav ? 'bottom-nav' : ''}">
<main class="{infiniteScrollPage ? 'infinite-scroll-page' : ''}">
<svelte:component this={child.component} {...child.props} />
</main>
Expand All @@ -13,6 +13,12 @@
.infinite-scroll-page {
min-height: 100vh;
}

.bottom-nav.main-content,
.bottom-nav .main-content {
padding-block-start: 0;
padding-block-end: var(--main-content-pad-top);
}
</style>
<script>
import { store } from './_store/store.js'
Expand Down
5 changes: 5 additions & 0 deletions src/routes/_pages/settings/general.html
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ <h2>{intl.media}</h2>

<h2>UI</h2>
<form class="ui-settings">
<label class="setting-group">
<input type="checkbox" id="choice-bottom-nav"
bind:checked="$bottomNav" on:change="onChange(event)">
{intl.bottomNav}
</label>
<label class="setting-group">
<input type="checkbox" id="choice-disable-custom-scrollbars"
bind:checked="$disableCustomScrollbars" on:change="onChange(event)">
Expand Down
8 changes: 8 additions & 0 deletions src/routes/_store/observers/bottomNavObservers.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export function bottomNavObservers (store) {
store.observe('bottomNav', bottomNav => {
// disables or enables the style
document.body.classList.toggle('bottom-nav', bottomNav);
document.querySelector('.main-content').classList.toggle('bottom-nav', bottomNav);
document.getElementById('main-nav').classList.toggle('bottom-nav', bottomNav);
Copy link
Owner

Choose a reason for hiding this comment

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

It's not clear to me why we need to set this in three different places.

Historically, the way I've done this is to toggle the entire <style> tag itself. You can see some examples here:

<style id="theGrayscaleStyle" media="only x">
/* Firefox doesn't seem to like applying filter: grayscale() to
* the entire body, so we apply individually.
*/
img, svg, video,
input[type="checkbox"], input[type="radio"],
.inline-emoji, .theme-preview {
filter: grayscale(100%);
}
</style>

export function grayscaleObservers (store) {
if (!process.browser) {
return
}
store.observe('enableGrayscale', enableGrayscale => {
const { instanceThemes, currentInstance } = store.get()
const theme = instanceThemes && instanceThemes[currentInstance]
style.setAttribute('media', enableGrayscale ? 'all' : 'only x') // disable or enable the style
switchToTheme(theme, enableGrayscale)
})
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed. i'll have to pull up my sleeves and try to get my local env running so i can clean this up

}, { init: false })
}
2 changes: 2 additions & 0 deletions src/routes/_store/observers/loggedInObservers.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { showComposeDialogObservers } from './showComposeDialogObservers.js'
import { badgeObservers } from './badgeObservers.js'
import { countryFlagEmojiPolyfill } from './countryFlagEmojiPolyfill.js'
import { centerNavObservers } from './centerNavObservers.js'
import { bottomNavObservers } from './bottomNavObservers.js'

// These observers can be lazy-loaded when the user is actually logged in.
// Prevents circular dependencies and reduces the size of main.js
Expand All @@ -23,6 +24,7 @@ export function loggedInObservers () {
notificationPermissionObservers()
customScrollbarObservers()
centerNavObservers()
bottomNavObservers()
customEmojiObservers()
showComposeDialogObservers()
badgeObservers()
Expand Down
2 changes: 2 additions & 0 deletions src/routes/_store/observers/observers.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ import { touchObservers } from './touchObservers.js'
import { grayscaleObservers } from './grayscaleObservers.js'
import { focusRingObservers } from './focusRingObservers.js'
import { leftRightFocusObservers } from './leftRightFocusObservers.js'
import { bottomNavObservers } from './bottomNavObservers.js'

export function observers (store) {
onlineObservers(store)
nowObservers(store)
navObservers(store)
bottomNavObservers(store);
pageVisibilityObservers(store)
resizeObservers(store)
touchObservers(store)
Expand Down
1 change: 1 addition & 0 deletions src/routes/_store/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const persistedState = {
currentRegisteredInstance: undefined,
// we disable scrollbars by default on iOS
disableCustomScrollbars: process.browser && /iP(?:hone|ad|od)/.test(navigator.userAgent),
bottomNav: true,
centerNav: false,
disableFavCounts: false,
disableFollowerCounts: false,
Expand Down
2 changes: 1 addition & 1 deletion src/scss/global.scss
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ body {

.main-content {
contain: content; // see https://www.w3.org/TR/2018/CR-css-contain-1-20181108/#valdef-contain-content
padding-top: var(--main-content-pad-top);
padding-block-start: var(--main-content-pad-top);
Copy link
Owner

Choose a reason for hiding this comment

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

Is this orthogonal to the PR? Does this have an effect with different languages? (I can't think of any bottom-to-top languages, but I'm sure they exist?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you could take it out 🤷


@supports not (contain: content) {
// For browsers which don't support the "contain" CSS property,
Expand Down