Refresh AdminLTE to 2.3.2 #104

dmpe · 2015-10-29T22:29:34Z

Hi @wch
Firstly, I like this package very much !! Thank you for it. Also, this will be a very important package for me in the near future and I will try to contribute to it.

This PR updates AdminLTE 2.1.2 from to 2.3.2 with changelog here: https://github.com/almasaeed2010/AdminLTE/releases. Yes, there were some additions, and they will need to be checked somehow (BTW: how do you test it ?).

Please, comment & I will try to fix it.
Thanks.

Just executed script -> didnt test

wch · 2015-10-30T14:38:01Z

Glad it's useful for you. Is there a particular reason you want to update to 2.3.2? I'm reluctant to update it without a lot of testing. Unfortunately, right now the testing is pretty much a manual process, checking all the examples in the documentation.

dmpe · 2015-10-31T20:53:49Z

Well, I already done that job of testing and I found no issues. Everything worked fine for me (rendering, functionality).
I put all the tests from man to its own folder and will create a big dashboard (hopefully) with all the elements in it.

add really big dashboard covering about 70-80% of all elements used or described. update news file as I believe version update to AdminLTE 2.3.2 is fine and not going to break anything.

dmpe · 2015-11-09T21:20:16Z

cc @wch Any update on this ?

add news and update descripition.

dmpe · 2015-12-06T19:51:22Z

Hi @wch ,
Given what I said, I might plan to add one or more features to it. Would you consider to review this PR, please?

Thank you very much.

wch · 2015-12-07T18:06:31Z

I just remembered that there were a number of changes that needed to be done manually to AdminLTE to make it work properly with shinydashboard. See:
https://github.com/rstudio/shinydashboard/blob/master/inst/AdminLTE/README-shiny-mods.md

The code changes are in this branch:
https://github.com/rstudio/AdminLTE/tree/shiny-mods

It's a little tricky to update AdminLTE, because we don't want to reintroduce those issues. It might be that most efficient way to deal with this is to for me to update AdminLTE myself and reapply the changes. Hopefully AdminLTE hasn't changed too much, and the commits on that branch can be re-applied to the AdminLTE 2.3.2 branch.

Regarding tests: it's not clear to me exactly how the tests in tests/ are meant to be used. Could you explain that? It would be helpful to have a detailed explanation of what each test is looking for.

In R, the automatic package check process sources files in the tests/ directory. But in this case, that's not really helpful, since the tests aren't automatic -- they require visual inspection, I assume. It would be better to put then in a directory called tests-manual and add that to .Rbuildignore.

dmpe · 2015-12-07T18:59:28Z

Regarding tests: it's not clear to me exactly how the tests in tests/ are meant to be used. Could you explain that? It would be helpful to have a detailed explanation of what each test is looking for.
In R, the automatic package check process sources files in the tests/ directory. But in this case, that's not really helpful, since the tests aren't automatic -- they require visual inspection, I assume. It would be better to put then in a directory called tests-manual and add that to .Rbuildignore.

~~Correct, the test are just there for the visual checking. So, that also a reason why there is if(interactive()){} code.~~

I just remembered that there were a number of changes that needed to be done manually to AdminLTE to make it work properly with shinydashboard. See:
https://github.com/rstudio/shinydashboard/blob/master/inst/AdminLTE/README-shiny-mods.md
The code changes are in this branch:
https://github.com/rstudio/AdminLTE/tree/shiny-mods
It's a little tricky to update AdminLTE, because we don't want to reintroduce those issues. It might be that most efficient way to deal with this is to for me to update AdminLTE myself and reapply the changes. Hopefully AdminLTE hasn't changed too much, and the commits on that branch can be re-applied to the AdminLTE 2.3.2 branch.

In https://github.com/rstudio/shinydashboard/blob/master/inst/AdminLTE/README-shiny-mods.md

Attached collapse click event handler to document, instead of to each collapse button. This is the same fix as ColorlibHQ/AdminLTE#304; if that is merged.....

From brief look, it seems to me to be fixed :: https://github.com/almasaeed2010/AdminLTE/blob/master/dist/js/app.js#L536-L549

In AdminLTE.css, the fonts are fetched from the local host, instead of from Google fonts.

Was that an issue before ? Because I could not tell the difference. But will need to look at it later.

dmpe · 2015-12-07T19:20:39Z

Fonts load for me, when running shinydash., from local PC.
I have renamed the folder and and will commit later.
Can't use renderMenu with menuSubItems #54 I cannot repro this. Maybe I am doing something wrong, but for me the second example can be expanded. Again, I believe it is fixed: https://github.com/almasaeed2010/AdminLTE/blob/master/dist/js/app.js#L391
Again, Collapsing of boxes not working with dynamic output #17 this is not reproducible for me too.

But, this is reproducible for me indeed: #42. I am not a javascript guru, but maybe if you could a look on https://github.com/almasaeed2010/AdminLTE/blob/master/dist/js/app.js#L557-L589 and tell if its now fixed by them ? (seems rather not)

At this stage, 2 out of 4 things are fixed as I believe. One is font thing, another one must be fixed. This sounds pretty good I think

chrisirhc · 2015-12-07T23:54:53Z

I was doing the same upgrade locally and I don't think #42 has been fixed by them as a user reported that bug after my update to 2.3.2 which led me to discover #42.

I'm not sure where the collapse/expand events are listened to by shiny though, couldn't find the source listening to that event in the shiny library.

dmpe · 2015-12-08T08:44:25Z

Most probably not. Thus I also assume #42 is really not fixed.

dmpe · 2015-12-08T13:52:10Z

@wch @chrisirhc Ok, now have commited the changes and the bug in #42 is fixed for me. This means Winston can only explain one thing (the fonts) and otherwise it is ready to be merged. 👏

Edit: Ping: @wch Maybe should I squash commits ?

dmpe · 2016-01-21T19:25:18Z

@wch Hi, please I know these days are hard due to finishing releases of Shiny and Rstudio but please, can you take a look on it again.
Thank you very much.

or maybe cc: @jcheng5 can look into this

wch · 2016-01-26T04:05:46Z

@dmpe Sorry, been busy with a lot of family and work things in the last couple months. It's a little challenging to assess some of these changes, but I'll give some feedback now.

wch · 2016-01-26T04:06:01Z

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: shinydashboard
 Title: Create Dashboards with 'Shiny'
-Version: 0.5.1
+Version: 0.5.2


Please use 0.5.1.9000.

wch · 2016-01-26T04:40:11Z

@dmpe, it looks pretty good overall - thanks for your attention to detail!

Here's my setup, for some context for the comments.

> devtools::session_info()
Session info -----------------------------------------------------------------------
 setting  value                       
 version  R version 3.2.3 (2015-12-10)
 system   x86_64, darwin13.4.0        
 ui       RStudio (0.99.868)          
 language (EN)                        
 collate  en_US.UTF-8                 
 tz       America/Chicago             
 date     2016-01-25                  

Packages ---------------------------------------------------------------------------
 package        * version date       source                            
 devtools       * 1.10.0  2016-01-23 CRAN (R 3.2.2)                    
 digest           0.6.9   2016-01-08 CRAN (R 3.2.3)                    
 htmltools        0.3     2015-12-29 CRAN (R 3.2.3)                    
 httpuv           1.3.3   2015-08-04 CRAN (R 3.2.0)                    
 jsonlite         0.9.19  2015-11-28 CRAN (R 3.2.2)                    
 magrittr         1.5     2014-11-22 CRAN (R 3.2.0)                    
 memoise          0.2.1   2014-04-22 CRAN (R 3.2.0)                    
 mime             0.4     2015-09-03 CRAN (R 3.2.0)                    
 R6               2.1.1   2015-08-19 CRAN (R 3.2.0)                    
 Rcpp             0.12.3  2016-01-10 CRAN (R 3.2.3)                    
 roxygen2         5.0.1   2015-11-11 CRAN (R 3.2.2)                    
 rsconnect        0.3.80  2015-05-25 Github (rstudio/rsconnect@b1f83e7)
 rstudioapi       0.5     2016-01-24 CRAN (R 3.2.3)                    
 shiny          * 0.13.0  2016-01-12 CRAN (R 3.2.3)                    
 shinydashboard * 0.5.2   <NA>       local                             
 stringi          1.0-1   2015-10-22 CRAN (R 3.2.0)                    
 stringr          1.0.0   2015-04-30 CRAN (R 3.2.0)                    
 withr            1.0.0   2015-09-23 CRAN (R 3.2.0)                    
 xtable           1.8-0   2015-11-02 CRAN (R 3.2.0)

wch · 2016-01-26T04:41:08Z

It would be helpful to have comments in the manual tests just what the manual test is looking for (if it's a specific issue).

delete `interactive()` because tests are in Rignore add comments fix tabItems issue comment repro issues fix version, bump both package further ahead and make sure that rstudio#42 is not repro on my pc

dmpe · 2016-01-26T16:08:56Z

inst/AdminLTE/README-shiny-mods.md

@@ -1,6 +1,4 @@
 This branch of AdminLTE contains the following changes from the stock version, to make it work better with Shiny.

-* Attached collapse click event handler to document, instead of to each collapse button. This is the same fix as https://github.com/almasaeed2010/AdminLTE/pull/304; if that is merged, then it should be possible to go to the stock version. Also see https://github.com/rstudio/shinydashboard/issues/17 for a test app.
 * The box collapse function triggers 'shown' and 'hidden' events, so that Shiny knows when outputs are visible or not. See https://github.com/rstudio/shinydashboard/issues/42 for a test app.


#42 not repro on my PC, (to delete)

wch · 2016-01-26T16:23:38Z

tests-manual/repro_issues_117.R

@@ -0,0 +1,190 @@
+library(shiny)


This example is way too complicated -- I'd hold off adding it until there's a minimal example, and when we understand what the problem actually is. In general, I'd like to minimize the number of additional changes in this pull request.

dmpe · 2016-01-26T16:32:06Z

Thanks Wiston for quick action on this again. At least, for (European) today I am done as i have turned off my Ubuntu VM where i do all the programming stuff.
This PR doesn add anything new, execpt for updating one library and move around the tests - so I dont think there are many changes.

You can either merge now or (European) tomorrow I would delete the #117 example from repository. I would also add some more comments as I have left some of them. And down-bump version of libraries.

and delete 117 test.

Refresh AdminLTE to 2.3.2

dmpe added 3 commits October 29, 2015 23:11

roxygenize + update description file

010fd8e

refresh adminLTE:

f0bf49f

Just executed script -> didnt test

grunt it

2c13843

dmpe added 2 commits October 31, 2015 22:18

add standalone tests so that one can use them in the tests folder

6d02858

add really big dashboard covering about 70-80% of all elements used or described. update news file as I believe version update to AdminLTE 2.3.2 is fine and not going to break anything.

fix tests for travis

ba350c6

dmpe added 2 commits December 6, 2015 18:19

add test to reproduce issue rstudio#110. Also upgrade shiny.

a11ea9c

add news and update descripition.

for skipping travis tests

4c0d024

dmpe added 2 commits December 8, 2015 14:38

commit after again applying fix for [rstudio#42], with after grunt

4dce160

merge selective commits from a different branch [skip ci]

d88beeb

wch reviewed Jan 26, 2016
View reviewed changes

add rstudio#117

8273914

delete `interactive()` because tests are in Rignore add comments fix tabItems issue comment repro issues fix version, bump both package further ahead and make sure that rstudio#42 is not repro on my pc

dmpe reviewed Jan 26, 2016
View reviewed changes

wch reviewed Jan 26, 2016
View reviewed changes

improve description file, and also add more comments for manual tests.

ebecad4

and delete 117 test.

wch added a commit that referenced this pull request Jan 28, 2016

Merge pull request #104 from dmpe/refresh

78899f7

Refresh AdminLTE to 2.3.2

wch merged commit 78899f7 into rstudio:master Jan 28, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refresh AdminLTE to 2.3.2 #104

Refresh AdminLTE to 2.3.2 #104

dmpe commented Oct 29, 2015 •

edited by bborgesr

Loading

wch commented Oct 30, 2015

dmpe commented Oct 31, 2015

dmpe commented Nov 9, 2015

dmpe commented Dec 6, 2015

wch commented Dec 7, 2015

dmpe commented Dec 7, 2015

dmpe commented Dec 7, 2015

chrisirhc commented Dec 7, 2015

dmpe commented Dec 8, 2015

dmpe commented Dec 8, 2015

dmpe commented Jan 21, 2016

wch commented Jan 26, 2016

wch Jan 26, 2016

wch commented Jan 26, 2016

wch commented Jan 26, 2016

dmpe Jan 26, 2016

wch Jan 26, 2016

dmpe commented Jan 26, 2016

Refresh AdminLTE to 2.3.2 #104

Refresh AdminLTE to 2.3.2 #104

Conversation

dmpe commented Oct 29, 2015 • edited by bborgesr Loading

wch commented Oct 30, 2015

dmpe commented Oct 31, 2015

dmpe commented Nov 9, 2015

dmpe commented Dec 6, 2015

wch commented Dec 7, 2015

dmpe commented Dec 7, 2015

dmpe commented Dec 7, 2015

chrisirhc commented Dec 7, 2015

dmpe commented Dec 8, 2015

dmpe commented Dec 8, 2015

dmpe commented Jan 21, 2016

wch commented Jan 26, 2016

wch Jan 26, 2016

Choose a reason for hiding this comment

wch commented Jan 26, 2016

wch commented Jan 26, 2016

dmpe Jan 26, 2016

Choose a reason for hiding this comment

wch Jan 26, 2016

Choose a reason for hiding this comment

dmpe commented Jan 26, 2016

dmpe commented Oct 29, 2015 •

edited by bborgesr

Loading