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

fork ui-menu and make active items clickable #1162

Merged
merged 3 commits into from
Apr 14, 2017

Conversation

indirectlylit
Copy link
Contributor

@indirectlylit indirectlylit commented Mar 29, 2017

Copies code from ui-menu into our code-base, and adds a new 'active' option.

Shorter-term, this provides a way to get back to the root level of the coach app after selecting a class, which is not currently possible with the state of the UI.

Longer-term, this also allows us greater control over this component and allows us to remove some unscoped files.

fixes #1160

@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #1162 into develop will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #1162   +/-   ##
========================================
  Coverage    73.79%   73.79%           
========================================
  Files          136      136           
  Lines         4358     4358           
  Branches       491      491           
========================================
  Hits          3216     3216           
  Misses        1080     1080           
  Partials        62       62

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1677a54...4d0a23c. Read the comment docs.

@indirectlylit
Copy link
Contributor Author

@christianmemije @rtibbles thoughts?

font-weight: bold
opacity: 1
.ui-menu-option__text

This comment was marked as spam.

This comment was marked as spam.

@christianmemije
Copy link
Contributor

Would be helpful if the options were links.

@christianmemije
Copy link
Contributor

Did you make any changes besides adding the 'active' option? If so can you point them out?

@indirectlylit
Copy link
Contributor Author

Would be helpful if the options were links.

Agreed. There's a lot of cleanup and modification we can do now with the code in our own repo.

Did you make any changes besides adding the 'active' option? If so can you point them out?

Nope. This PR is broken into two main commits:

  • a706e24 is a simple port of the components, with no functional changes. Just enough editing to pass our linters.
  • 4d0a23c is where the active option was added.

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

This works, can do further clean up as we turn these things into links!

@rtibbles rtibbles merged commit b557039 into learningequality:develop Apr 14, 2017
@indirectlylit indirectlylit deleted the nav-port branch April 23, 2017 17:29
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.

current nav-bar link is not clickable
4 participants