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(sidebar): add support for customize sidebar #165

Merged
merged 2 commits into from
Jan 22, 2017
Merged

Conversation

neoFelhz
Copy link
Collaborator

@neoFelhz neoFelhz commented Jan 21, 2017

What kind of change does this PR introduce? (check one with "x")

  • Bug fix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Does this PR introduce a breaking change? (check one with "x")

  • Yes
  • No

Description

Add support for sidebar customize.
You can customize your sidebar icons in config file now.
(I remove the I'm Feeling Lucky item as well)


Verification steps

  • You have to update your _config.yml in order to use this feature and avoid bug.

- Remove I'm Feeling Lucky
- Support for sidebar customize
- Change the config file
</li>
-->
<% if(theme.sidebar.homepage === true) { %>
<li id="sidebar-first-li">
Copy link
Collaborator

@pidupuis pidupuis Jan 21, 2017

Choose a reason for hiding this comment

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

  • Please fix indentation (remove 4 spaces before lines 18, 19 and 20)

<% if(theme.sidebar.archives === true) { %>
<li class="dropdown">
<a href="#" class="ripple-effect dropdown-toggle" data-toggle="dropdown">
<i class="material-icons sidebar-material-icons"><%= theme.sidebar.archive.icon %></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • It should be theme.sidebar.archives.icon (you forgot an 's')
  • Please surround this line with the following condition: if(theme.sidebar.archives.icon){


<!-- Categories -->
<% if(theme.sidebar_categories === true) { %>
<% if(theme.sidebar.categories === true) { %>
<li class="dropdown">
<a href="#" class="ripple-effect dropdown-toggle" data-toggle="dropdown">
<i class="material-icons sidebar-material-icons">chrome_reader_mode</i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • You seem to have forgotten to use the icon from the configuration like <%= theme.sidebar.categories.icon %>
  • And then surround this line with the following condition: if(theme.sidebar.categories.icon){

<a href="<%= theme.dropdown[i].link %>" target="_blank" title="<%= i %>">
<i class="material-icons sidebar-material-icons sidebar-indent-left1pc-element"><%= theme.dropdown[i].icon %></i>
<a href="<%= theme.sidebar.dropdown[i].link %>" target="_blank" title="<%= i %>">
<i class="material-icons sidebar-material-icons sidebar-indent-left1pc-element"><%= theme.sidebar.dropdown[i].icon %></i>
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Please surround this line with the following condition: if(theme.sidebar.dropdown[i].icon){

@neoFelhz
Copy link
Collaborator Author

neoFelhz commented Jan 21, 2017

@pidupuis OK,I will fix themw.

- add a check use 'if' whether user do configure icon
- add some missed lines (about categories icon)
@neoFelhz
Copy link
Collaborator Author

@pidupuis Now I push a new commit to fix them,please review them again(And sorry for my carelessness)

@pidupuis pidupuis merged commit fb07933 into canary Jan 22, 2017
@pidupuis pidupuis deleted the feat/sidebar branch January 22, 2017 15:22
@iblh iblh mentioned this pull request Jan 28, 2017
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.

None yet

3 participants