-
Notifications
You must be signed in to change notification settings - Fork 297
Barbara/bookmark #187
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
Barbara/bookmark #187
Conversation
5505a99
to
386506c
Compare
tools/package.json
Outdated
"grunt": "^1.0.1", | ||
"grunt-contrib-concat": "^1.0.1", | ||
"grunt-contrib-cssmin": "^2.0.0", | ||
"grunt-contrib-jshint": "^1.1.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can remove this line, and then re-run yarn.
tools/package.json
Outdated
@@ -1,10 +1,14 @@ | |||
{ | |||
"devDependencies": { | |||
"babel-eslint": "^7.1.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed.
R/dashboardSidebar.R
Outdated
# just passed through (as the `data-value` attribute) to the | ||
# `dashboardPage()` function | ||
tags$aside( | ||
class = paste("main-sidebar"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
paste()
isn't necessary.
R/dashboardSidebar.R
Outdated
@@ -391,6 +397,10 @@ menuItem <- function(text, ..., icon = NULL, badgeLabel = NULL, badgeColor = "gr | |||
) | |||
} | |||
|
|||
dataExpanded <- shiny::restoreInput(id = "itemExpanded", default = "") %OR% "" # prevent this from being NULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about sidebarItemExpanded
instead of itemExpanded
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment above here explaining what's going on would be helpful.
}, | ||
// needed so we set the appropriate value for bookmarked apps on startup | ||
initialize: function(el) { | ||
$(this).trigger('change'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary? If so, a more detailed comment would be helpful.
Also, I think it should be be $(el)
instead. $(this)
shouldn't return anything useful.
var $expanded = $(el).find('li ul.menu-open'); | ||
if ($expanded.length === 1) { | ||
return $expanded.find('a').attr('href').substring(1); | ||
} else if ($(el).attr("data-expanded")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be needed anymore.
if (value !== null) { | ||
var firstChild = 'a[href="#' + value + '"]'; | ||
var $ul = $(firstChild).parent().parent('.treeview-menu'); | ||
$ul.addClass('menu-open'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be simpler in the long run to check for the menu-open
class, and if it's not present, trigger a click event on the $(firstChild).parent().prev()
(or whatever it is), which will cause the event handler from AdminLTE to run. Then you don't have to try to duplicate the behavior, like calling $ul.show()
.
And same for the else
below.
srcjs/input_binding_tabItem.js
Outdated
setValue: function(el, value) { // eslint-disable-line consistent-return | ||
var self = this; | ||
var anchors = $(el).find('li:not(.treeview)').children('a'); | ||
anchors.each(function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe try putting // eslint-disable-line consistent-return
here?
33b26c1
to
9d67d62
Compare
…lete yarn dependencies
…e more verbose (and redundant) "collpased"/"expanded"
…s to sidebarMenu.Rd)
…e first tab, if no other explicit choice is made
9d67d62
to
1b5b56b
Compare
closing in favor of #199 |
Simple example app (open, collapse the sidebar, bookmark, then open the bookmarked URL - the sidebar will be collapsed):
The next two apps are (in some version) included in
/tests-manual
.Complex example:
Even more complex example (dynamic sidebarMenu):