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

Colin/output styles #438

Open
wants to merge 31 commits into
base: main
Choose a base branch
from
Open

Conversation

ColinFay
Copy link
Contributor

This PR implements a new way to customize the outputs from both {learnr} and {gradethis} using exercise options.

@ColinFay
Copy link
Contributor Author

R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/feedback.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/options.R Outdated Show resolved Hide resolved
R/options.R Outdated Show resolved Hide resolved
@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 7, 2020

@trestletech @schloerke @dcossyleon

As of our last discussion, this is now the default behavior:

  • Run Code always output the console
  • If this output is an error, a colored square is shown
  • Submit answer can use specific colors for the output, and the feedback and console output can be both turned off (default is display them both)

=> {learnr}: http://connect.thinkr.fr/learnr-colored-output/
=> {gradethis}: http://connect.thinkr.fr/learnr-colored-output-gradethis/

@trestletech
Copy link
Contributor

Looking good to me! @dcossyleon I can't remember where we netted out in the last meeting, but I notice that in the current demo learnr exercise syntax errors have their output highlighted in an alert background. I think that's helpful since we don't have the ability to print error text in right like we could in the IDE. But I remember you had reservations. Did we decide we did or did not want that?

R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/feedback.R Outdated Show resolved Hide resolved
@schloerke
Copy link
Collaborator

The code changes look great! Just a few minor changes left. 😄

@dcossyleon
Copy link

Looking good to me! @dcossyleon I can't remember where we netted out in the last meeting, but I notice that in the current demo learnr exercise syntax errors have their output highlighted in an alert background. I think that's helpful since we don't have the ability to print error text in right like we could in the IDE. But I remember you had reservations. Did we decide we did or did not want that?

Originally I had suggested that "Run Code" always give the standard black output, but I agree now that some sort of color indication of an error would indeed be helpful. The colored background feels too bulky for "Run Code" error output, but a nice compromise would be the suggestion of using the colored outline (see below).

This is subtle enough that the focus remains on the text in the error message, while still giving a color indication that something went awry. I think the colored background should be saved for gradethis cases only, where human feedback is also being shown and the student is expecting a "louder" indication of success or failure.

image

@dcossyleon
Copy link

For gradethis output styles:

Output that gets wrapped in <pre> tags, like the example, below looks great!

Screen Shot 2020-10-07 at 11 40 32 AM

Could we apply the same spacing (and border removal) that separates the text and output for other output types (tables, images, plots, etc.)?

screenshots

@ColinFay
Copy link
Contributor Author

@dcossyleon this is implemented in the last commit:

Screenshot 2020-10-13 at 10 51 22
Screenshot 2020-10-13 at 10 51 37

ColinFay and others added 2 commits October 13, 2020 11:46
@dcossyleon
Copy link

Looks great to me

R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
R/exercise.R Outdated Show resolved Hide resolved
ColinFay and others added 3 commits October 14, 2020 09:15
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
Co-authored-by: Barret Schloerke <barret@rstudio.com>
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

5 participants