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

Add an XBlock student_view to XModule #477

Merged
merged 1 commit into from
Jul 24, 2013

Conversation

cpennington
Copy link
Contributor

This just wraps XModule.get_html, and ignores javascript and css for
now.

[LMS-189]

@dianakhuang @brianhw @ormsbee: Review?


class Module(XModule):
def get_html(self):
return '<input type="hidden" class="schematic" name="{item_id}" height="480" width="640">'.format(item_id=self.item_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we getting rid of this module? Its it just not getting used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not in setup.py, so no one could have used it, and it's broken as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anything else that we can get rid of, using the same criterion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so.

@brianhw
Copy link
Contributor

brianhw commented Jul 23, 2013

Just one typo, and otherwise looks good.


# Test that for all of the leaf XModule Descriptors,
# the student_view wrapper returns the same thing in its content
# as get_html returns
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't these docstrings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I didn't want them to muddle the test names

@dianakhuang
Copy link
Contributor

I think that's my only comment. Otherwise, it looks good.


Makes no use of the context parameter
"""
return Fragment(unicode(self.get_html()))
Copy link
Contributor

Choose a reason for hiding this comment

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

What's coming out of self.get_html() exactly? Because if it's a unicode object, this call to unicode() seems redundant. And if it's a UTF-8 byte string, it seems like you'd have to pass in the encoding so it doesn't default to ASCII?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm. Good point. It's coming out as unicode in the lms, because we're telling mako to render it that way. So I'll remove that call.

This just wraps XModule.get_html, and ignores javascript and css for
now.

[LMS-189]
@ormsbee
Copy link
Contributor

ormsbee commented Jul 24, 2013

👍

cpennington added a commit that referenced this pull request Jul 24, 2013
Add an XBlock student_view to XModule
@cpennington cpennington merged commit dd338c1 into master Jul 24, 2013
@cpennington cpennington deleted the cale/xmodule-student-vieww branch July 24, 2013 19:08
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
…ission_names

Allowing filesubmissions to specify which files are allowed and require that certain files are present.
idegtiarov pushed a commit to Code-Institute-Org/edx-platform that referenced this pull request Sep 17, 2018
…-accounts-tab

Added checking information  before show tabs
xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request Aug 19, 2019
fix(underscore): move underscore to theme did not work
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.

4 participants