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

Add side navigation variant with icons on the left #2932

Merged
merged 3 commits into from Mar 18, 2020

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented Mar 17, 2020

Done

Adds side navigation variant with icons on the left.

Fixes #2862

QA

  • Run ./run or demo
  • Make sure side nav works correctly with right side icons
  • Make sure side nav default variant is not affected
  • Make sure there are docs describing the use of variant with icons
    • please note CodePen example will show unstyled side nav, because it was not published yet

Screenshot 2020-03-17 at 14 38 34

@bartaz bartaz added the Feature 🎁 New feature or request label Mar 17, 2020
@webteam-app
Copy link

@lyubomir-popov
Copy link
Contributor

Nice work, one suggestion:

can we clip the border? @spencerbygraves what do you think?
image

Copy link
Contributor

@lyubomir-popov lyubomir-popov left a comment

Choose a reason for hiding this comment

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

@bartaz Approved with one additional mixin ehnancement proposal, feel free to ignore if you think it is overkill.

position: relative;
}

// nested 2nd level of navigation
.p-side-navigation__item .p-side-navigation__item & {
padding-left: 2 * $sph-inner;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the levels should be configurable when you invoke the mixin; many sites won't need 3 levels, and some might need 5; my proposal: @mixin p-side-navigation($depth: 3)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feels a bit overkill.

The design was discussed many times and it seems for now we barely have examples that would use all 3 level of navigation. If anything really needs more nesting we could add it later.

Also nesting is just 2 not very long rules (4 precisely if you count --icons variant), so there is no much benefit in file size to reduce it to just one.

The complexity of adding configurable mixin that probably no one will use doesn't seem worth it. It doesn't seem like we do anything like this anywhere else.

While flexibility and ability to configure things is great it comes with cost of maintenance and ease of use.

I guess the biggest problem we are trying to solve with Vanilla is to keep our products consistent, so lets start with good default variants of components. If anything different is needed we can consider it on case by case basis.

@spencerbygraves
Copy link

can we clip the border?

@lyubomir-popov to draw attention to the text more than the icon?

I've just noticed that in the design I made the hr #e5e5e5 rather than #cdcdcd, but I guess that would be a change to the hr everywhere?

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Mar 17, 2020

@spencerbygraves yes, to reenforce the text keyline.
I also prefer #e5e5e5, but it feels as something we need to define at a more global level. Some borders need to be more prominent than others:

then there are many lines that could be lighter:

  • accordion separators
  • table rows after the first one
  • separators inside list-like patterns in general

@bartaz
Copy link
Contributor Author

bartaz commented Mar 17, 2020

@spencerbygraves For the border I used #cdcdcd as this is our light theme border color. If possible we should keep this consistent, so either keep that or update to #e5e5e5 everywhere where light theme borders are used.

And what do you say about border width? Do we want to shorten it to align with text as per @lyubomir-popov suggestion?

@spencerbygraves
Copy link

@bartaz @lyubomir-popov ok, let's keep the border as it is, it would be good to look at it's use as you mention above.

Happy to try it aligned with the text. Would we need to bear this in mind for tables?

Thanks

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Mar 17, 2020

No implications for tables, but could be a nice example to follow in the contextual menu pattern (if / when we add an example with icons).

Added a new issue about border colours, to revisit when we have spare time. It is something we've discussed on a number of occasions so feels like we should make an official proposal:
#2933

@bartaz bartaz merged commit c3ca7d5 into canonical:master Mar 18, 2020
@bartaz bartaz deleted the side-navigation-icons branch March 18, 2020 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build optional sidenav icons
4 participants