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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Start using the HtmlUtils safe template functions #11856

Closed
wants to merge 1 commit into from
Closed

Start using the HtmlUtils safe template functions #11856

wants to merge 1 commit into from

Conversation

andy-armstrong
Copy link
Contributor

@andy-armstrong andy-armstrong commented Mar 20, 2016 •
edited

This change starts to convert the platform to use the new HtmlUtils class. I've primarily converted the Teams UI, along with the shared helper classes and Underscore templates that they depend upon.

Sandbox

Testing

  • Unit, integration, acceptance tests as appropriate

Reviewers

If you've been tagged for review, please check your corresponding box once you've given the 馃憤.

FYI: @AlasdairSwan @dan-f @dsjen @bjacobel

Post-review

  • Squash commits

@andy-armstrong
Copy link
Contributor Author

This is a new version of https://github.com/edx/edx-platform/pull/11799 that is rebased against master.

@andy-armstrong andy-armstrong force-pushed the andya/add-ui-toolkit branch 2 times, most recently from 11a408d to 36eda99 Compare March 22, 2016 02:21
@andy-armstrong
Copy link
Contributor Author

@robrap @cahrens I've addressed all of your feedback from the previous PR. Note that this is currently pointing at a GitHub hash until the UI Toolkit changes are approved and a release is cut.

robrap
robrap reviewed Mar 22, 2016
View reviewed changes
common/static/common/js/components/views/feedback_notification.js
capitalCamel = _.compose(str.capitalize, str.camelize);
// create Notification.Warning, Notification.Confirmation, etc
var capitalCamel, intents;
capitalCamel = _.compose(str.capitalize, str.camelize);
intents = ["warning", "error", "confirmation", "announcement", "step-required", "help", "mini"];
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to match indent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I'd tried to revert my changes in this file, but PyCharm fooled me as I had it in 'ignore whitespace' mode. I've fully reverted them now.

robrap
robrap reviewed Mar 22, 2016
View reviewed changes
lms/static/js/student_profile/views/learner_profile_fields.js
showNotificationMessage: function() {
var accountSettingsPageUrl = this.options.accountSettingsPageUrl,
accountSettingsLinkHtml = HtmlUtils.joinHtml(
HtmlUtils.HTML('<a href="' + accountSettingsPageUrl + '">'),
Copy link
Contributor

Choose a reason for hiding this comment

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

You should have an inner joinHtml() for this as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems strange, because we are then implicitly HTML escaping a URL.

@robrap
Copy link
Contributor

robrap commented Mar 22, 2016

@andy-armstrong The code looks clean. Thanks!

I was tired when I reviewed, so I should double check. This is a big review. I dropped a bunch of comments that don't necessarily need to get refactored, but it would be nice to use them to help settle best practices to be documented.

@andy-armstrong andy-armstrong mentioned this pull request Mar 22, 2016
Merged
5 tasks
andy-armstrong
andy-armstrong reviewed Mar 22, 2016
View reviewed changes
lms/djangoapps/teams/static/teams/js/views/topic_card.js
)));
render: function() {
var teamCount = this.model.get('team_count');
HtmlUtils.setHtml(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robrap Here's an example where we use HtmlUtils.setHtml with a string. It seems useful for updating simple paragraph and span tags. I couldn't find an example for HtmlUtils.append but it seems reasonable to support it for consistency.

Copy link
Contributor

Choose a reason for hiding this comment

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

In our best practice guide, would we want people to use this.$el.text(...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a very good point. I'll switch it...

@andy-armstrong andy-armstrong changed the title Add UI Toolkit to the platform Update teams to use the new HtmlUtils safe template functions Mar 22, 2016
@andy-armstrong andy-armstrong changed the title Update teams to use the new HtmlUtils safe template functions Start using the HtmlUtils safe template functions Mar 22, 2016
@robrap
Copy link
Contributor

robrap commented May 9, 2016

This is a general message being posted for all Safe Templates PRs. If you are making steady progress, sorry about this, and thank you!

We need to get any Safe Template PRs merged as soon as possible.

Make sure you have the latest copy of ./scripts/safe_template_linter.py and ./scripts/safe-commit-linter.sh, and make sure that ./scripts/safe-commit-linter.sh reports 0 violations for this PR.

Feel free to ping me with any questions, and/or refer to the Safe Template Linter documentation.

Thanks!

@cahrens
Copy link

cahrens commented May 16, 2016

@andy-armstrong should this PR be closed?

@andy-armstrong
Copy link
Contributor Author

@cahrens No, I need to get back to merging this.

@robrap
Copy link
Contributor

robrap commented May 16, 2016

@andy-armstrong FYI: When you rebase from master you'll get reports of violations for this PR in Jenkins under quality.

@andy-armstrong andy-armstrong force-pushed the andya/add-ui-toolkit branch 4 times, most recently from 8017ad6 to 1b2b82e Compare May 16, 2016 20:03
@andy-armstrong andy-armstrong mentioned this pull request Jun 17, 2016
Merged
6 tasks
@cahrens
Copy link

cahrens commented Aug 2, 2016

@andy-armstrong OK to close this PR?

@robrap
Copy link
Contributor

robrap commented Aug 2, 2016

@cahrens @andy-armstrong: I don't think so. @scottrish will be reviewing the remaining safe template open PRs for next steps (prompted by the branch removal request).

What would it take to land this work?

@andy-armstrong
Copy link
Contributor Author

@robrap @cahrens I'll try rebasing it tonight and see how far off it seems.

@andy-armstrong andy-armstrong force-pushed the andya/add-ui-toolkit branch 2 times, most recently from a1e72d8 to 3631498 Compare August 3, 2016 19:53
@andy-armstrong andy-armstrong deleted the andya/add-ui-toolkit branch October 12, 2016 03:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@andy-armstrong @robrap @cahrens