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

Feature request: Ability to use dividers in side navigation #865

Closed
xylish7 opened this issue Jun 2, 2020 · 3 comments · Fixed by #885
Closed

Feature request: Ability to use dividers in side navigation #865

xylish7 opened this issue Jun 2, 2020 · 3 comments · Fixed by #885

Comments

@xylish7
Copy link
Contributor

xylish7 commented Jun 2, 2020

Summary

I want to be able to add a divider between the links of the side navigation

Justification

The designers which with I work requested this feature.

Desired UX and success metrics

The dividers should look like those presented in the following image:

image

Must-have functionality

  • no must-have functionality at the moment

Specific timeline issues / requests

Needed to use it in a future release of the website I work on

Available extra resources

I will gladly develop it taking in consideration your ideas on what is the best way to do it.

@xylish7
Copy link
Contributor Author

xylish7 commented Jun 4, 2020

@vpicone @jnm2377 I've implemented a way to add the divider to side navigation.

I thought that adding the divider from from the nav-items.yaml (also as @jnm2377 said) is an easy thing to do for those who want to use this feature.

I went for level 1 dividers because I'm not sure if also using dividers in a sub nav item will look ok. So this is how it will look inside nav-items.yaml:

- title: Demo
  pages:
    - path: /demo
- title: Gallery
  pages:
    - path: /gallery
  hasDivider: true
- title: Contributions
  pages:
    - path: /contributions

Then in the LeftNavItem.js I checked for the hasDivider prop:

if (items.length === 1) {
          return (
            <>
              <SideNavLink
                onClick={closeLeftNav}
                icon={<span>dummy icon</span>}
                element={Link}
                className={cx({
                  [styles.currentItem]: isActive,
                })}
                isActive={isActive}
                to={`${items[0].path}`}
              >
                {category}
              </SideNavLink>
              {hasDivider && <hr className={styles.divider} />}
            </>
          );
        }

Also added the divider for list items which has level 2 links:

return (
          <>
            <SideNavMenu
              icon={<span>dummy icon</span>}
              isActive={isActive} // TODO similar categories
              defaultExpanded={isActive}
              title={category}
            >
              <SubNavItems
                onClick={closeLeftNav}
                items={items}
                pathname={pathname}
              />
            </SideNavMenu>
            {hasDivider && <hr className={styles.divider} />}
          </>
        );

Let me know what you think and if the approach seems ok I can also make a pull request so you can check all the code 🙂.

@xylish7
Copy link
Contributor Author

xylish7 commented Jun 19, 2020

@jnm2377 @vpicone It passed almost 3 weeks and no one came with another solution. Should I proceed with a PR which contains the solution presented above?

@vpicone
Copy link
Collaborator

vpicone commented Jun 22, 2020

So sorry for the delay! I think this is a really elegent design and well implemented, nice work!! Definitely open a PR.

In general, issues can fall through the cracks unless they're prioritized. Once you have a solid approach you feel good about, a PR is a great next step. Also if things ever fall by the wayside, don't hesitate to ping the aux chat. We get a lot of GitHub tags and they can be lost in the shuffle.

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 a pull request may close this issue.

2 participants