-
Notifications
You must be signed in to change notification settings - Fork 8
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
Modernize XBlock for Hawthorn #26
Conversation
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.
@stvstnfrd, first round of review done.
Course Staff | ||
~~~~~~~~~~~~ | ||
|
||
To add a full-screen image to your course: | ||
|
||
- upload the image file onto your course's Files & Uploads page | ||
|
||
- note: you can skip this step if you've already uploaded the image | ||
elsewhere, e.g.: S3. | ||
|
||
- copy the URL on that page | ||
- go to a unit in Studio | ||
- select "Image Modal XBlock" from the Advanced Components menu | ||
|
||
|image-cms-add| | ||
|
||
You can now edit and preview the new component. | ||
|
||
|image-cms-view| | ||
|
||
Using the Studio editor, you can edit the following fields: | ||
|
||
- display name | ||
- image URL | ||
- thumbnail URL (defaults to image URL, if not specified) | ||
- description (useful for screen readers, longer descriptions) | ||
- alt text (useful for screen readers, captions, tags; displays when image does not) | ||
|
||
|image-cms-editor-1| | ||
|image-cms-editor-2| | ||
|
||
|
||
Participants | ||
~~~~~~~~~~~~ | ||
|
||
|image-lms-view-normal| | ||
|
||
Click on the image to zoom in full-screen. | ||
|
||
|image-lms-view-zoom| | ||
|
||
Click on the image again to zoom out. | ||
|
||
Click and drag to pan around. | ||
|
||
`View a demo of the CMS`_ | ||
|
||
`View a demo of the LMS`_ | ||
|
||
|
||
.. |badge-coveralls| image:: https://coveralls.io/repos/github/Stanford-Online/xblock-image-modal/badge.svg?branch=master | ||
:target: https://coveralls.io/github/Stanford-Online/xblock-image-modal?branch=master | ||
.. |badge-travis| image:: https://travis-ci.org/Stanford-Online/xblock-image-modal.svg?branch=master | ||
:target: https://travis-ci.org/Stanford-Online/xblock-image-modal | ||
.. |image-cms-add| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/cms-add.jpg | ||
:width: 100% | ||
.. |image-cms-advanced-module-list| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/advanced-module-list.png | ||
:width: 100% | ||
.. |image-cms-editor-1| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-editor-1.png | ||
:width: 100% | ||
.. |image-cms-editor-2| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-editor-2.png | ||
:width: 100% | ||
.. |image-cms-settings-menu| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/settings-menu.png | ||
:width: 100% | ||
.. |image-cms-view| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-view.png | ||
:width: 100% | ||
.. |image-lms-view-normal| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/lms-view-normal.png | ||
:width: 100% | ||
.. |image-lms-view-zoom| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/lms-view-zoom.png | ||
:width: 100% | ||
.. _View a demo of the CMS: https://youtu.be/IcbGYfbav2w | ||
.. _View a demo of the LMS: https://youtu.be/0mpjuThDoyE | ||
.. https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/xblock-image-modal-demo-lms.mov |
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 needs to be removed/changed, it's for image modal.
Course Staff | |
~~~~~~~~~~~~ | |
To add a full-screen image to your course: | |
- upload the image file onto your course's Files & Uploads page | |
- note: you can skip this step if you've already uploaded the image | |
elsewhere, e.g.: S3. | |
- copy the URL on that page | |
- go to a unit in Studio | |
- select "Image Modal XBlock" from the Advanced Components menu | |
|image-cms-add| | |
You can now edit and preview the new component. | |
|image-cms-view| | |
Using the Studio editor, you can edit the following fields: | |
- display name | |
- image URL | |
- thumbnail URL (defaults to image URL, if not specified) | |
- description (useful for screen readers, longer descriptions) | |
- alt text (useful for screen readers, captions, tags; displays when image does not) | |
|image-cms-editor-1| | |
|image-cms-editor-2| | |
Participants | |
~~~~~~~~~~~~ | |
|image-lms-view-normal| | |
Click on the image to zoom in full-screen. | |
|image-lms-view-zoom| | |
Click on the image again to zoom out. | |
Click and drag to pan around. | |
`View a demo of the CMS`_ | |
`View a demo of the LMS`_ | |
.. |badge-coveralls| image:: https://coveralls.io/repos/github/Stanford-Online/xblock-image-modal/badge.svg?branch=master | |
:target: https://coveralls.io/github/Stanford-Online/xblock-image-modal?branch=master | |
.. |badge-travis| image:: https://travis-ci.org/Stanford-Online/xblock-image-modal.svg?branch=master | |
:target: https://travis-ci.org/Stanford-Online/xblock-image-modal | |
.. |image-cms-add| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/cms-add.jpg | |
:width: 100% | |
.. |image-cms-advanced-module-list| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/advanced-module-list.png | |
:width: 100% | |
.. |image-cms-editor-1| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-editor-1.png | |
:width: 100% | |
.. |image-cms-editor-2| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-editor-2.png | |
:width: 100% | |
.. |image-cms-settings-menu| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/settings-menu.png | |
:width: 100% | |
.. |image-cms-view| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/studio-view.png | |
:width: 100% | |
.. |image-lms-view-normal| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/lms-view-normal.png | |
:width: 100% | |
.. |image-lms-view-zoom| image:: https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/lms-view-zoom.png | |
:width: 100% | |
.. _View a demo of the CMS: https://youtu.be/IcbGYfbav2w | |
.. _View a demo of the LMS: https://youtu.be/0mpjuThDoyE | |
.. https://s3-us-west-1.amazonaws.com/stanford-openedx-docs/xblocks/image-modal/static/images/xblock-image-modal-demo-lms.mov |
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.
Yes, I haven't done much on the README
; I wanted to get eyes on the code first.
} | ||
|
||
function pre_submit() { | ||
problem_progress.text('(Loading...)') | ||
problem_progress.text('(Loading...)'); |
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 did we miss this?
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 suspect you'd find a number of unknown issues on any code you're writing and not running a linter on.
As it were, semicolons are actually largely optional in JS and only need to be required in a few circumstances.
This is a one-line function that behaves identically w/out the punctuation.
Anyway, this is why we should always lint code.
color: #3399cc; | ||
font-weight: 600; | ||
padding-bottom: 20px; | ||
padding-top: 20px; |
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.
color: #3399cc; | |
font-weight: 600; | |
padding-bottom: 20px; | |
padding-top: 20px; | |
padding-top: 20px; | |
padding-bottom: 20px; | |
color: #3399cc; | |
font-weight: 600; |
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'm guessing that your comments throughout this file are a request to group properties by type?
Side note: The new Github PR "Suggested Change" feature is nice, but comments be still often be useful;
it's not obvious what you're asking for here.
Anyway, assuming it's the grouping, that's not prescribed by our style guide nor edx's.
It's also not enforced by any linter of which I'm aware.
I think you made a similar comment on another XBlock PR, but our linter actually does alphabetize properties, without concern to type grouping.
While there some general support for property grouping, I don't believe it's a standard (like pylint/pycodestyle) and since none of our groups enforce it and neither does our linter, I don't think we should make these changes.
border: 3px solid #cccccc; | ||
height: 120px; | ||
max-width: 100%; | ||
padding: 5px; | ||
width: 100%; |
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.
border: 3px solid #cccccc; | |
height: 120px; | |
max-width: 100%; | |
padding: 5px; | |
width: 100%; | |
width: 100%; | |
height: 120px; | |
max-width: 100%; | |
padding: 5px; | |
border: 3px solid #cccccc; |
.reset_button { | ||
font-weight: 600; | ||
height: 40px; | ||
vertical-align: middle; | ||
} | ||
.hint_button { | ||
font-weight: 600; | ||
height: 40px; | ||
vertical-align: middle; | ||
} | ||
.submit_button { | ||
font-weight: 600; | ||
height: 40px; | ||
vertical-align: middle; | ||
} |
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.
.reset_button { | |
font-weight: 600; | |
height: 40px; | |
vertical-align: middle; | |
} | |
.hint_button { | |
font-weight: 600; | |
height: 40px; | |
vertical-align: middle; | |
} | |
.submit_button { | |
font-weight: 600; | |
height: 40px; | |
vertical-align: middle; | |
} | |
.reset_button, | |
.hint_button, | |
.submit_button { | |
height: 40px; | |
vertical-align: middle; | |
font-weight: 600; | |
} |
color: #666; | ||
display: inline-block; | ||
font-size: 1em; | ||
font-weight: 100; | ||
padding-left: 5px; |
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.
color: #666; | |
display: inline-block; | |
font-size: 1em; | |
font-weight: 100; | |
padding-left: 5px; | |
display: inline-block; | |
padding-left: 5px; | |
color: #666; | |
font-size: 1em; | |
font-weight: 100; |
color: #666; | ||
font-style: italic; | ||
margin-top: 8px; |
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.
color: #666; | |
font-style: italic; | |
margin-top: 8px; | |
margin-top: 8px; | |
color: #666; | |
font-style: italic; |
def _convert_to_int(value_string): | ||
""" | ||
Convert a string to integer | ||
|
||
Default to 0 | ||
""" | ||
try: | ||
value = int(value_string) | ||
except ValueError: | ||
value = 0 | ||
return value |
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.
Can this not get moved out to a utilities file or something?
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 we typically don't split out helpers if they're only used in one place (this is marked as a private method, not public).
As mentioned previously, I'll eventually be creating stanford-xblock-utils
library and if this is used by other things, we can consider moving it then.
Closing for now, since this never landed. I've pulled a copy of the branch into my fork, so maybe I'll revisit some day :) https://github.com/stvstnfrd/xblock-submit-and-compare/tree/modernize |
No description provided.