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

Feature/begin add python qs #7

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

Conversation

ssl9ek
Copy link

@ssl9ek ssl9ek commented Mar 2, 2019

Taking a crack at adding in some python questions. Not sure if I made updates to all the right places for adding in a new question pool.

I haven't set up angular on my own computer so I haven't tested it or checked how the formatting will look (tried to use &#10; for newline. perhaps should switch to <br> ).

@kirkbrunson I know you're not a ML4T student anymore. I'm hoping you can spend a bit of time to take a look and see if it works! Thanks!

@ssl9ek ssl9ek mentioned this pull request Mar 2, 2019
Copy link
Owner

@kirkbrunson kirkbrunson left a comment

Choose a reason for hiding this comment

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

Thanks for your work Serena! Having python questions will be a great addition here. Two things:

  • Minor typo leading to build error
  • Let's make sure that formatting question code displays well in the browser. Currently a number of them display like this.

finance_questions: any = JSON.parse(JSON.stringify(finance_questions));
questions: Array<any> = [this.ml_questions.concat(this.finance_questions), finance_questions, questions, []]
python_questions: any = JSON.parse(JSON.stringify(python_questions));
questions: Array<any> = [this.ml_questions.concat(this.finance_questions.concat(this.python_questions), finance_questions, ml_questions, python_questions, []]
Copy link
Owner

Choose a reason for hiding this comment

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

Missing ) to close concat at column 105 leads to a build error

Suggested change
questions: Array<any> = [this.ml_questions.concat(this.finance_questions.concat(this.python_questions), finance_questions, ml_questions, python_questions, []]
questions: Array<any> = [this.ml_questions.concat(this.finance_questions.concat(this.python_questions)), finance_questions, ml_questions, python_questions, []]

@kirkbrunson
Copy link
Owner

@ssl9ek Parsing and adding the python questions look good! Can you update how they are displayed in app.component.html so that they are formatted well?

@ssl9ek
Copy link
Author

ssl9ek commented Mar 25, 2019

Thanks for screencap-ing the display. I was wondering how the newline formats would turn out. Could try replacing &#10 with <br> to see if that would be a quick fix? Otherwise I can take a look at app.component.html

Thanks for pointing out the build errors.

@kirkbrunson
Copy link
Owner

Can you do a local npm install and build so that you can see how it displays? If so, find/replace &#10 would be fast to try, but I think we'll need code formatting.

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.

None yet

2 participants