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

Adjustment practice #2

Open
wants to merge 55 commits into
base: master
Choose a base branch
from
Open

Adjustment practice #2

wants to merge 55 commits into from

Conversation

Jiayue-He
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@shravanisamala shravanisamala left a comment

Choose a reason for hiding this comment

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

Hi Jiayue-He! Good job changing the ui.R. and server.R files to one app.R file! Just a few general things to consider:

  • I added many comments regarding the style because I was comparing the app side-by-side to the style guide. Make sure you go back in and add any necessary titles and remove and unnecessary details like the colons, strong(), and h4()s for bullet points.
  • Be sure to go through the whole code and add white space around = signs and other symbols where needed.
  • I would ask Dr. Neil and/or Mr. Carey about the huge chunks of code that are commented out and see if you can delete them.
  • Change the order of the tabs and fix the go buttons
  • Cite the packages you haven't cited yet. Check if all of them are used with the stale packages function we talked about today
  • Make sure your code is explicit everywhere and by this I mean the whole session = session, inputId = "" and so on
  • And for the overall code, I would try to go back when you have free time and fix the alignment and indenting of the code. I noticed that sometimes each line of text underneath the previous line was not consistently indented. For example, some lines are indented twice or thrice while some of the later lines are indented once. I would change them all to one just so we do not go past that 80 character guideline.

Overall, great job! You did a lot of great work to get the app to where it is now! I hope some of my comments will help!


library(dplyr)
library(shinycssloaders)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing the BoastUtils package! When I tried running this code on my laptop, it says that the script function which is in BoastUtils was not found.


source("helpers.R")

ui <- dashboardPage(
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the style guide, I believe this should be:

ui <- list(
dashboardPage( ...

menuItem("References", tabName = "references", icon = icon("leanpub"))
),

tags$div(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is the updated PSU button code from the style guide!

tags$div(
class = "sidebar-logo",
boastUtils::sidebarFooter()
)

app.R Outdated
tabItems(
#Adding pre-requisites page to remove background from instructions page

tabItem(tabName="prereq",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The tab items are out of order. I think it should be Overview page, then prerequisites page

app.R Outdated
######### Could be combined but left separate so easily understood#####################

dashboardBody(
tags$head(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure what lines 61 - 113 are supposed to do so I tried commenting it out and the app's background color for each page remained white! Consider asking Dr. Neil or Mr. Carey if it is ok to completely delete these lines of code.

app.R Outdated
"Carey, R. (2019). boastUtils: BOAST Utilities, R Package.
Available from https://github.com/EducationShinyAppTeam/boastUtils"
)
) #end of tabItem
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't forget the copyright info at the end of the references page!

    br(),
    br(),
    br(),
    boastUtils::copyrightInfo()



}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check bracket alignment

app.R Outdated

observeEvent(input$restart,{
updateButton(session, "submit", disabled = FALSE)
updateButton(session,"restart",disable =FALSE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

disabled not disable

app.R Outdated
}
else{
if(length(index_list$list) == 1){
updateButton(session, "nextq", disabled = TRUE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

session = session etc.

app.R Outdated

})

renderIcon()
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried commenting this out and the app works without it. Was this something you were working on or can it be deleted?

DESCRIPTION Outdated
Comment on lines 19 to 20
URL: https://psu-eberly.shinyapps.io/Sample_App
BugReports: https://github.com/EducationShinyAppTeam/Sample_App/issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

The website for URL and BugReports should be updated to say Logistic_Regression instead of Sample_App.

app.R Outdated
Comment on lines 8 to 15
#library(discrimARTs)
library(leaflet)
library(raster)
library(DT)

library(RColorBrewer)
#library(car)
#library(rgdal)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could delete the lines that have unused packages instead of leaving them commented out.

app.R Outdated
where data is divided into recommended 10 groups. The p-value can
determine the significance of the result.")),
br(),
h4(tags$li("Hosmer-Lemeshow Test Statstics")),
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a typo in Statistics here.

app.R Outdated
\\over exp_{ij}}$$')),
br(),
br(),
div(style = "text-align: center",bsButton("start","Go to the overview",
Copy link
Collaborator

Choose a reason for hiding this comment

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

After you update the order of tabs to be first overview and then prerequisites, the label on this button should be changed to Explore.

app.R Outdated
br(),
div(style = "text-align: center",bsButton("start","Go to the overview",
icon("bolt"),style = "danger",
size = "large",class="circle grow"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

None of the buttons should have the class = circle grow argument.

app.R Outdated
Comment on lines 268 to 269
tableOutput("lemeshowDF"),
tableOutput("obsexpDF"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was getting errors on the page where these outputs were supposed to be shown, the packages ResourceSelection and data.table should be added at the top of the app so that these outputs will run properly.

app.R Outdated
),
#set continue button
div(style = "text-align: center",
bsButton(inputId = "go1", label = "Play the game!", icon("bolt"), style= "danger", size= "large", class='circle grow')
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could change the label on this button to just say "Play!".

app.R Outdated
Comment on lines 330 to 332
selectInput(inputId="MedYvar", label="Select Response Variable Y",
choices = c("Acceptance"),
selected = 'Acceptance'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this selection is needed since there is only one option for the response variable for each of the possible data sets.

app.R Outdated
Comment on lines 368 to 369
mainPanel(
plotOutput("empericalLogitPlot", width = "100%")%>% withSpinner(color="#ffa500")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This plot wouldn't load until I added the Stat2Data package at the beginning of the code.

Copy link
Collaborator

@lbednarczyk lbednarczyk left a comment

Choose a reason for hiding this comment

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

Good job overall! Other than the comments that I left throughout the code, I would just make sure to go through the code and fix spacing issues and make all of the arguments explicit like Shravani said. Also make sure to add the missing packages so that everything runs smoothly.

@rpc5102
Copy link
Member

rpc5102 commented Jan 4, 2022

6aaa28d was deployed to shinyapps.io to resolve an SSL Expiry error although I suggest further work to clean up.

@neilhatfield neilhatfield requested a review from Maxinewsu May 27, 2022 20:54
Copy link
Collaborator

@Maxinewsu Maxinewsu left a comment

Choose a reason for hiding this comment

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

Recommended Priority: Urgent

  • Issues are all addressed.
  • The link of URL and Bugreports on DESCRIPTION is not correct
  • Missing screenshot on README.
  • Add a citation on the overview page.
  • Add a line "This app was last updated by xxx" at the bottom of the overview page.
  • Switch the order of Prerequisite and Overview pages.
  • The last three referenes on Reference page are not cited using the required format.
  • The font size is too big compared to other shiny apps.

@Maxinewsu Maxinewsu requested review from a team and removed request for a team June 9, 2022 23:51
Sburke26 added 4 commits June 12, 2023 09:48
"Warning: glm.fit: fitted probabilities numerically 0 or 1 occurred" may not be fixable the way the app is made.
@Sburke26 Sburke26 requested a review from tarynmch June 27, 2023 16:46
Copy link

@tarynmch tarynmch left a comment

Choose a reason for hiding this comment

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

Good work with updating the app. The UI looks good, but in the server, I noticed that some of the IDs are not camelcase (like gamescore), some things are not individually indented (line 749 and 755), the chapter name should be changed, and there is no alt text for some of the plots and images in the game. The only thing I suggest is to go through the question bank excel and make the capitalization consistent for the choices.
I also noticed that when using the Single Logistic Regression sliders, it said in the console, "glm.fit: fitted probabilities numerically 0 or 1 occurred."

Made fixes to code including the addition of alt texts, naming of objects, indentation, and deletion of unused code.

Made updates to the question bank file.

Removed unused images.

Fixed Chapter in description
@Sburke26
Copy link

Thank you for the review.
I made additional fixes to the app.
The "glm.fit: fitted probabilities numerically 0 or 1 occurred." message can't be removed due to the current way the data is simulated.
I think I added alt text to everything but the logistic plot which uses renderPlotly() instead of renderPlot().
Please let me know if you notice anything else.

@Sburke26 Sburke26 requested a review from tarynmch July 11, 2023 13:33
Copy link

@tarynmch tarynmch left a comment

Choose a reason for hiding this comment

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

Looks good. The main thing I noticed was that when you click game it says, "Select your answer from below" but after you click next it says, "pick an answer from below." I would just make sure that is concise. Then make sure randNum is camelcase. I also suggest that for the game, if you want to get to 20 points within 10 questions, you could add a question counter. Otherwise, the app looks great!

@Sburke26
Copy link

Thank you for the review. I made some changes and added a question counter. Please let me know if you see anything else.

@Sburke26 Sburke26 requested a review from tarynmch July 21, 2023 18:34
tarynmch
tarynmch previously approved these changes Aug 1, 2023
Copy link

@tarynmch tarynmch left a comment

Choose a reason for hiding this comment

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

Looks good! The only thing I suggest is that on the Overview page in the instructions, be sure to mention that the New Sample button is within the Explore tab. Otherwise, be sure to get rid of the unnecessary code in the app.R before you merge the branches. Great work.

Sburke26 and others added 9 commits August 7, 2023 11:47
Remove an problematic install.packages command
Cleaning up the explore page to improve efficiency and table accessibility
Remove unnecessary packages and code from the app; deleted helpers file that contained elements counter to the Style Guide.
Clean up of alt text
Reorganizing Game Server code
Initial work towards reformatting the game page of the app
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.