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

Restore jqueryUI accordion functionality removed by Canvas commit. #264

Merged
merged 2 commits into from
Aug 11, 2017

Conversation

patchin
Copy link

@patchin patchin commented Aug 11, 2017

This is a workaround which restores jqueryUI accordion functionality removed by the following commit: instructure@44ed07d

Taken from: 'https://github.com/ryankshaw/widgetize-canvas-lms-user-content'

@grahamb grahamb requested review from va7map and grahamb August 11, 2017 22:43
Copy link
Member

@va7map va7map left a comment

Choose a reason for hiding this comment

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

One concern is this loads an external script that we have to trust. Would it be safer if we hosted our own copy of the drop-in replacement code?

@@ -6,6 +6,10 @@

*/

!function(s,d,url,e,p){
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here to explain what this is doing here, for future us?

Copy link
Author

Choose a reason for hiding this comment

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

Ok

@grahamb
Copy link
Member

grahamb commented Aug 11, 2017

@va7map unpkg.com is trustworthy, I don't have a problem with pulling it from there (it's a CDN for npm modules). Bonus is that https://unpkg.com/widgetize-canvas-lms-user-content redirects to the most recent published version, so we don't need to keep it up to date.

Copy link
Member

@va7map va7map left a comment

Choose a reason for hiding this comment

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

Ah, good to know! 🌈

Copy link
Member

@grahamb grahamb left a comment

Choose a reason for hiding this comment

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

Just add a comment in before the block stating what it is for and we're good to go.

@grahamb grahamb merged commit 8d61f0f into sfu:sfu-develop Aug 11, 2017
@grahamb
Copy link
Member

grahamb commented Aug 11, 2017

Thanks @patchin. Go ahead and upload to prod.

@patchin
Copy link
Author

patchin commented Aug 11, 2017

Ok thanks!

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.

3 participants