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

Vueified channel list page #1264

Merged
merged 44 commits into from
Mar 22, 2019
Merged

Conversation

jayoshih
Copy link
Contributor

@jayoshih jayoshih commented Feb 26, 2019

Description

Vueified channel list page

Issue Addressed (if applicable)

#900
#721
#1258

Steps to Test

Implementation Notes (optional)

Does this introduce any tech-debt items?

I have added a TODO: REMOVE BACKBONE comment wherever we can remove our dependency on backbone

As for testing, I was unable to check if a modal has properly been opened (we're still using bootstrap modals). Also, I wasn't able to mock jquery requests as it interferes with backbone's models. I added TODOs in each of these locations

Also, it might be worth considering adding a central studioVue instance to avoid a lot of repetitive Vue.use(Vuex) code

Checklist

  • Is the code clean and well-commented?
  • Have the changes been added to the CHANGELOG?
  • Are all user-facing strings translated properly (if applicable)?
  • Are all UI components LTR and RTL compliant (if applicable)?

@jayoshih jayoshih requested a review from rtibbles February 26, 2019 19:32
@micahscopes
Copy link
Contributor

This is a prolific PR!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Some things that could be cleaned up, I think. Have not manually tested yet.

]),
{
channelStrings() {
return channelStrings;
Copy link
Member

Choose a reason for hiding this comment

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

This pattern has cropped up on Kolibri too - seems like it is probably worth my while thinking about how to make $trs work for mixins.

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 would be great!

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Code looks good, all issues addressed. Manual walk through, everything is functional and looks as expected.

@rtibbles rtibbles merged commit 6dad17d into learningequality:develop Mar 22, 2019
@jayoshih jayoshih deleted the vue-channel-list branch May 28, 2019 18:29
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