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 component with default styling #2915

Merged
merged 16 commits into from Mar 16, 2020

Conversation

bartaz
Copy link
Contributor

@bartaz bartaz commented Mar 11, 2020

Done

  • adds basic version of side navigation
  • adds examples and simple docs

Fixes #2860
Fixes #2861

QA

Screenshots

Screenshot 2020-03-12 at 10 13 11

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

@bartaz bartaz force-pushed the side-navigation branch 2 times, most recently from a8dfd78 to 35747d3 Compare March 12, 2020 07:09
@bartaz bartaz marked this pull request as ready for review March 12, 2020 09:19
@bartaz bartaz changed the title WIP: Side navigation Add side navigation component with default styling Mar 12, 2020
@spencerbygraves
Copy link

@bartaz we've just had our weekly product design review which produced some interesting feedback (thanks @cristinadresch), which I've just discussed with @lyubomir-popov.

Can we try updating the selected states please, to simplify things a bit:

  • Remove the bold and darker style on items you have previously selected (this does feel a bit confusing when looking at the current version)
  • Remove the bold style on the currently selected items (this would help to differentiate selected items from the section titles, and bring it in line with tabs)

Now that we can see it in action, we may need to tweak things like this to get it right, hope that's ok.

This shows the difference, on the left is what we have now:

image

Thanks.

@bartaz
Copy link
Contributor Author

bartaz commented Mar 12, 2020

@spencerbygraves Sure, this will simplify the code as well and remove a need for confusing additional 'selected' state on non active items.

@spencerbygraves
Copy link

@bartaz @anthonydillon @lyubomir-popov if you look at the charmhub side nav, you'll see functionality to toggle on/off filters for the main content. Should we add that to the Vanilla side nav at some point?

https://charmhub-io-canonical-web-and-design-pr-5.run.demo.haus/

The side nav here will be updated with the Vanilla one.

@bartaz
Copy link
Contributor Author

bartaz commented Mar 13, 2020

@spencerbygraves Couple of notes on that:

  • charmhub seems to be using custom colors of the side nav (and they have it on gray background) - we currently assume white background and use light theme colors, if we want project to be able to change colors only in sidenav component without affecting rest of light theme we need separate variables for that
  • side nav is going to have support for right hand icons, this should cover the X case for filters - the easiest way would be to have X always visible (when item is highlighted), having it appear on hover would require a special case

So, I think what we have in plan to implement should be allow for such functionality (with some small compromises). We could later iterate on improvements if needed.

@lyubomir-popov
Copy link
Contributor

Would it be possible to keep the focus outline above the black bar?
image

@bartaz
Copy link
Contributor Author

bartaz commented Mar 13, 2020

Would it be possible to keep the focus outline above the black bar?

@lyubomir-popov I'll have a look - I just used existing mixins. I'll check if we can control it.

@bartaz
Copy link
Contributor Author

bartaz commented Mar 16, 2020

@lyubomir-popov So it seems currently the only way to display outline above our absolutely positioned border is to hide the border on focus.

I'm personally not sure if it's better - looks a bit like border is "disappearing" rather than getting covered by outline:

side-nav-focus

But will leave that design decision to you - do we keep current default (dark border over outline) or should we implement workaround and hide border on focus?

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.

LGTM!

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 nested sidenav levels Build basic sidenav styles
4 participants