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

[debatable] Hide left sidebar if no navitems present #5149

Merged

Conversation

pascalwengerter
Copy link
Contributor

Description

Felt surprisingly easy, I'd vote in favor of only hiding the navigation (and not the whole sidebar) to keep the iconic CI blue plus OC logo present for public links though...

@pascalwengerter pascalwengerter requested a review from kulmann May 25, 2021 15:36
@@ -220,6 +220,10 @@ export default {
return 'uk-visible@l'
},

isSidebarItems() {
return this.sidebarNavItems.length > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add

if (this.sidebarNavItems.length === 0) {
    return false
}

to isSidebarVisible instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would like to move towards early returns in code style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code is updated to reflect this 🤓

@pascalwengerter pascalwengerter force-pushed the 25052021_hide-left-sidebar-if-no-navitems branch 2 times, most recently from fb16acc to d8eab2e Compare May 25, 2021 16:34
@pascalwengerter pascalwengerter force-pushed the 25052021_hide-left-sidebar-if-no-navitems branch from d8eab2e to e4287f8 Compare May 25, 2021 16:35
Copy link
Contributor

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

LGTM. I agree that it's not an ideal solution regarding usability. But an empty left sidebar feels even weirder, so I would do it this way until we have the changed layout with the two left sidebar.

@pascalwengerter pascalwengerter merged commit 9221a99 into a11y-swarming May 25, 2021
@delete-merged-branch delete-merged-branch bot deleted the 25052021_hide-left-sidebar-if-no-navitems branch May 25, 2021 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants