-
Notifications
You must be signed in to change notification settings - Fork 69
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
Sidebar redesign #200
Sidebar redesign #200
Conversation
add an issue to pr please. #174? |
src/frontend/js/modules/sidebar.js
Outdated
*/ | ||
init(settings, moduleEl) { | ||
this.nodes.sections = Array.from(moduleEl.querySelectorAll('.' + Sidebar.CSS.section)); | ||
this.nodes.sections.forEach(section => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls, move the handler to the separate method
src/frontend/js/modules/sidebar.js
Outdated
handleSectionTogglerClick(sectionId, sectionEl, togglerEl, event) { | ||
event.preventDefault(); | ||
this.collapsed[sectionId] = !this.collapsed[sectionId]; | ||
window.localStorage.setItem(LOCAL_STORAGE_KEY, JSON.stringify(this.collapsed)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls, move the methods for working with the storage to the separate utility class,
like this (rough example):
class Storage {
constructor(prefix){
}
setItem(name, value){}
getItem(name){}
}
class SidebarStorage extends Storage {
constructor(){
super('sidebar-state')
this.keyName = 'sections-collapsed'
}
set(value){
super.setItem(this.keyName, value)
}
get(){
return super.getItem(this.keyName)
}
}
src/frontend/js/modules/sidebar.js
Outdated
* @param {HTMLElement} sectionEl - element of the section to toggle | ||
* @param {HTMLElement} togglerEl - section's toggler button element | ||
* @param {boolean} collapsed - new collapsed state | ||
* @param {boolean} animated - true if state should change with animation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {boolean} animated - true if state should change with animation | |
* @param {boolean} [animated] - true if state should change with animation |
src/frontend/js/modules/sidebar.js
Outdated
* @param {boolean} animated - true if state should change with animation | ||
*/ | ||
setSectionCollapsed(sectionEl, togglerEl, collapsed, animated = true) { | ||
togglerEl.classList.toggle(Sidebar.CSS.togglerCollapsed, collapsed); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the parent collapsed class at the CSS?
.section--collapsed .toggler {}
.docs-sidebar { | ||
border-bottom: 1px solid var(--color-line-gray); | ||
box-sizing: border-box; | ||
padding: 30px 22px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--layout-padding-vertical
--layout-padding-horizontal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--layout-padding-horizontal
is 16px now
width: 100vw; | ||
|
||
@media (--desktop) { | ||
width: 344px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use variable please
margin: 0; | ||
z-index: 1; | ||
position: relative; | ||
max-height: 1000px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add will-change: max-height
please
margin: 0; | ||
z-index: 1; | ||
position: relative; | ||
max-height: 1000px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why there is a 1000px hardcoded?
|
||
{% set mainClass = 'docs-sidebar' %} | ||
|
||
<div data-module="sidebar" class="{{mainClass}}__wrapper"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong usage of BEM: Element should not include its Block
<docs-sidebar__wrapper>
<docs-sidebar/>
</docs-sidebar>
The Block should include Elements instead
<docs-sidebar>
<docs-sidebar__toggler/>
<docs-sidebar__body/>
</docs-sidebar>
@@ -0,0 +1,54 @@ | |||
|
|||
{% set mainClass = 'docs-sidebar' %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need this variable if it's almost unused?
{{ svg('aside-logo') }} | ||
</div> | ||
<div class="docs-sidebar__logo-caption"> | ||
<span>Powered by CodeX Docs</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
span looks redundant
src/frontend/js/modules/sidebar.js
Outdated
* Creates base properties | ||
*/ | ||
constructor() { | ||
this.nodes = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Describe in jsdoc, please
src/frontend/js/modules/sidebar.js
Outdated
this.nodes = {}; | ||
const storedState = window.localStorage.getItem(LOCAL_STORAGE_KEY); | ||
|
||
this.collapsed = storedState ? JSON.parse(storedState) : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can pick up a more understandable name?
src/frontend/js/modules/sidebar.js
Outdated
init(settings, moduleEl) { | ||
this.nodes.sections = Array.from(moduleEl.querySelectorAll('.' + Sidebar.CSS.section)); | ||
this.nodes.sections.forEach(section => { | ||
const id = section.getAttribute('data-id'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const id = section.getAttribute('data-id'); | |
const id = section.dataset.id; |
@@ -70,6 +72,13 @@ | |||
background: color-mod(var(--color-link-active) blackness(+10%)); | |||
} | |||
} | |||
|
|||
--squircle { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool
if we implement the change you suggest there's going to be strange behaviour when you collapse current section and reload page - section will expand again. I have no idea how to resolve this, so, I suggest not doing this change now |
i don't know why i may need to use site with all collapsed sections. anyway it is just an idea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works cool 👍
src/frontend/styles/vars.pcss
Outdated
@@ -27,10 +27,12 @@ | |||
--layout-padding-vertical: 30px; | |||
--layout-width-aside: 300px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this variable still actual?
width: var(--sidebar-width); | ||
|
||
@media (--desktop) { | ||
--sidebar-width: 344px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it would be better to move it to the vars.pcss
under the --layout-sidebar-width
?
&__content { | ||
border-bottom: 1px solid var(--color-line-gray); | ||
box-sizing: border-box; | ||
padding: var(--layout-padding-vertical) 22px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the --layout-padding-horizontal
please, and update its value to 22
Updates docs sidebar styles. Top level pages become collapsable, collapsed sections state is saved between page reloads.