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

Implemented Vue.js in the editShow.mako template. #3849

Merged
merged 70 commits into from
Apr 22, 2018

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Mar 10, 2018

  • Created select-list vue component
  • Created anidbReleaseGroupUi vue component.
  • Created dedicated css files for the vue components in css/vue
  • Added aliases setter to series class.
  • Added series aliases to apiv2.
  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

Todo:

  • Migrate language select
  • Migrate quality chooser
  • vue save modal / confirmation

Need to update the flatten_folders part:

                                    <input type="checkbox" id="season_folders" name="flatten_folders" ${'checked="checked"' if show.flatten_folders == 0 or app.NAMING_FORCE_FOLDERS else ''} ${'disabled="disabled"' if app.NAMING_FORCE_FOLDERS else ''}/> group episodes by season folder (uncheck to store in a single folder)

Known bugs:

  • When adding a custom release group to the Release Groups (the middle box). And then moving it to blacklist or whitelist, it's not saved. My suggestion. Do not allow it to be moved to the middle box.
  • Adding custom release groups, will make it check anidb, to get a short release group name. This takes some time, and it makes the api call timeout. The timeout is supposedly 10s, but that's not correct/working.

OmgImAlexis and others added 4 commits March 1, 2018 07:48
* Created select-list vue component
* Created anidbReleaseGroupUi vue component.
* Created dedicated css files for the vue components in css/vue
* Added aliases setter to series class.
* Added series aliases to apiv2.
@p0psicles
Copy link
Contributor Author

@OmgImAlexis let's try to use this setup for components for now.

@@ -0,0 +1,52 @@
<script type="text/x-template" id="anidb-release-group-ui">
<div id="anidb_release_group_ui_wrapper" class="top-10">
<link rel="stylesheet" type="text/css" href="css/vue/anidbreleasegroupui.css?${sbPID}" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it make more sense to inline the CSS since we want a single vue file later on?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

<div class="col-sm-4 left-whitelist" >
<span>Whitelist</span>
<ul>
<li v-for="item in releaseWhitelist">{{ item }}</li>
Copy link
Collaborator

@OmgImAlexis OmgImAlexis Mar 10, 2018

Choose a reason for hiding this comment

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

We should be using properly named vars for in loops.
e.g. release in releaseWhitelist

data() {
return {
// JS only
defSeries: '12345',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default values should be DEFAULTS not placeholders. Set this to 0 if you want it to be a Number field with no value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't need the series id here. so it's still a left-over. I'll remove it.

},
methods: {
sendValues: function() {
this.$emit('change', this.editItems);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is this.editItems?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Has been refactored in the mean time. Please have another look.

@@ -0,0 +1,64 @@
<script type="text/x-template" id="select-list">
<div class="select-list">
<link rel="stylesheet" type="text/css" href="css/vue/selectlistui.css?${sbPID}" />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again inline CSS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return items;
}
},
transformToIndexedObject: arrayOfStrings => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest switching this to a map function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

didn't know you could also inject the index into the map callback. Will do!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

indexerName: $('#indexer-name').attr('value'),
config: MEDUSA.config,
exceptions: exceptions,
seriesObj: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please switch this to series. No need to describe the type in the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

scene: false,
sports: false,
paused: false,
location: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Single quotes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

},
saveSeries: async function(subject) {
if (['seriesObj', 'all'].includes(subject)) {
const data = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Switch this to using deconstructing like I showed in slack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, still need to do this one

};
},
onChangeIgnoredWords: function(items) {
console.log('Event from child component emitted', items);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either remove or switch all of the console.log to console.debug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

},
onChangeAliases: function(items) {
console.log('Event from child component emitted', items);
this.seriesObj.config.aliases = items.map(item => item.value);
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should all be setup using a watcher on the local data instead of an event handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do I create a watcher for this? What's the item I need to watch here?

}
}
});
$('[v-cloak]').removeAttr('v-cloak');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't need this if you mount the main vue instance to vue-wrap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* Changed anidbReleaseGroupUi.mako component, to accept a list of object.
* Added saving the black/white lists.
* Added getters/setters to the Series class for blacklist / whitelist.
* Enabled dogpile for caching the short_group_names() function.
this.releaseGroups = this.allGroups;
this.createIndexedObjects(this.blacklist, 'blacklist');
this.createIndexedObjects(this.whitelist, 'whitelist');
this.createIndexedObjects(this.allGroups, 'releasegroups');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not allow passing an array?

p0psicles and others added 5 commits March 11, 2018 13:33
anidbReleaseGroupUi:
* added the removal of release groups
* prevent adding groups to white/black list, if there already there.

* selectListUi: Replaced the loop with a map.
* added eventBus to be able to use the file browser together with Vue.
* Replaced console.log with console.debug.
* Renamed seriesObj to series.
else:
short_group_list = groups
return short_group_list
#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: need to remove this

@@ -0,0 +1,177 @@
<style scoped>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used scoped, don't know if thats a problem

Copy link
Contributor

@sharkykh sharkykh Apr 11, 2018

Choose a reason for hiding this comment

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

No browser actually supports that according to https://caniuse.com/#feat=style-scoped
Correction, FF supports that, but not by default.
vue-loader supports that but it's not in here?


// We need this hack, to send the location back to the vue model.
// As no event is triggered, Vue doesn't know to update it's value.
$.eventBus.$emit('locationEvt', currentBrowserPath);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OmgImAlexis the eventBus i was talking about.

…nvert-editshow-to-vue

# Conflicts:
#	themes-default/slim/views/editShow.mako
#	themes/dark/templates/editShow.mako
#	themes/light/templates/editShow.mako

Added app-link and file-browser components to editShow.
<input type="text" name="location" id="location" value="${show._location}" class="form-control form-control-inline input-sm input350"/>
<input type="hidden" name="indexername" id="form-indexername" :value="indexerName" />
<input type="hidden" name="seriesid" id="form-seriesid" :value="seriesId" />
<file-browser name="location" ref="locationBtn" id="location" v-model="location" class="form-control form-control-inline input-sm input350"></file-browser>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OmgImAlexis I've tried implementing it. But it's showing the part of the div out of place:
image

Any idea?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll have to check but I think I added classes to the input so you don't need them on the <file-browser>. Check the component file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tnx that was it.

p0psicles and others added 4 commits March 22, 2018 18:14
* Reverted back to <a> on the vue-components/file-browser.mako.
…nvert-editshow-to-vue

# Conflicts:
#	medusa/helpers/__init__.py
#	themes-default/slim/static/js/edit-show.js
#	themes-default/slim/views/editShow.mako
#	themes/dark/assets/js/browser.js.map
#	themes/dark/assets/js/core.js.map
#	themes/dark/assets/js/edit-show.js
#	themes/dark/assets/js/edit-show.js.map
#	themes/dark/templates/editShow.mako
#	themes/light/assets/js/browser.js.map
#	themes/light/assets/js/core.js.map
#	themes/light/assets/js/edit-show.js
#	themes/light/assets/js/edit-show.js.map
#	themes/light/templates/editShow.mako
@@ -13,7 +13,7 @@
@click="fileClicked(file)"
>
<span :class="'ui-icon ' + (file.isFile ? 'ui-icon-blank' : 'ui-icon-folder-collapsed')"></span> {{file.name}}
</app-link>
</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you meant to change the opening tag to a as well.

…nvert-editshow-to-vue

# Conflicts:
#	themes-default/slim/views/addShows_newShow.mako
#	themes-default/slim/views/layouts/main.mako
#	themes-default/slim/views/vue-components/saved-message.mako
#	themes/dark/templates/addShows_newShow.mako
#	themes/dark/templates/layouts/main.mako
#	themes/dark/templates/vue-components/saved-message.mako
#	themes/light/templates/addShows_newShow.mako
#	themes/light/templates/layouts/main.mako
#	themes/light/templates/vue-components/saved-message.mako
…x-template' into feature/convert-editshow-to-vue

# Conflicts:
#	themes-default/slim/views/editShow.mako
#	themes-default/slim/views/vue-components/quality-chooser.mako
#	themes/dark/templates/editShow.mako
#	themes/dark/templates/vue-components/quality-chooser.mako
#	themes/light/templates/editShow.mako
#	themes/light/templates/vue-components/quality-chooser.mako
For not it's saving as a combined allowed/preferred quality.
But the api has been prepared to save them independent.
@p0psicles
Copy link
Contributor Author

Need to update the api tests.

'apiParams': {
'language': indexer['api_params']['language'],
'useZip': indexer['api_params']['use_zip'],
'session': '',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is session needed if it's always empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no we can remove that

@@ -0,0 +1,21 @@
window.vueSubmitForm = async function(formId) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest renaming so we don't get it mixed up with Vue plugins which are also mounted on window and start with "vue".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did I change this? Can't remember doing anything with vue-submit-form.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sharkykh did you add it somewhere? Then maybe I rebased it into here?

Copy link
Contributor

@sharkykh sharkykh Apr 19, 2018

Choose a reason for hiding this comment

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

This is from the quality PR
It was there when I rebased
edit: 2fb1227

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep the mess to a minimum. we can fix these in #3818. And then i'll merge it back into this one.

@@ -77,7 +77,7 @@ Set checked shows/episodes to <select name="newStatus" class="form-control form-
<% series_id = str(cur_series[0]) + '-' + str(cur_series[1]) %>
<tr id="${series_id}">
<th><input type="checkbox" class="allCheck" data-indexer-id="${cur_series[0]}" data-series-id="${cur_series[1]}" id="allCheck-${series_id}" name="${series_id}-all" checked="checked" /></th>
<th colspan="2" style="width: 100%; text-align: left;"><app-link indexer-id="${cur_series[0]}" class="whitelink" href="home/displayShow?indexername=indexer-to-name&seriesid=${cur_series[1]}">${show_names[(cur_series[0], cur_series[1])]}</app-link> (${ep_counts[(cur_series[0], cur_series[1])]})
<th colspan="2" style="width: 100%; text-align: left;"><app-link data-indexer-to-name="${cur_series[0]}" class="whitelink" href="home/displayShow?indexername=indexer-to-name&seriesid=${cur_series[1]}">${show_names[(cur_series[0], cur_series[1])]}</app-link> (${ep_counts[(cur_series[0], cur_series[1])]})
Copy link
Collaborator

@OmgImAlexis OmgImAlexis Apr 19, 2018

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's keep the mess to a minimum. we can fix these in #3818. And then i'll merge it back into this one.

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 spoke too soon. Apparently it's changed in this branch, because of all the rebases.

OmgImAlexis
OmgImAlexis approved these changes Apr 19, 2018
Copy link
Collaborator

@OmgImAlexis OmgImAlexis left a comment

Choose a reason for hiding this comment

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

I've noticed a few of the app-links have been changed.

They should also be using the placeholder function of app-link.

Copy link
Collaborator

@OmgImAlexis OmgImAlexis left a comment

Choose a reason for hiding this comment

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

I've noticed a few of the app-links have been changed.

They should also be using the placeholder function of app-link.

@p0psicles
Copy link
Contributor Author

I'm doing one more rebase. But then we need to get this merged asap.

…nvert-editshow-to-vue

# Conflicts:
#	themes-default/slim/static/js/add-show-options.js
#	themes-default/slim/views/editShow.mako
#	themes/dark/assets/js/add-show-options.js
#	themes/dark/assets/js/add-show-options.js.map
#	themes/dark/templates/editShow.mako
#	themes/light/assets/js/add-show-options.js
#	themes/light/assets/js/add-show-options.js.map
#	themes/light/templates/editShow.mako
OmgImAlexis
OmgImAlexis previously approved these changes Apr 19, 2018
…nvert-editshow-to-vue

# Conflicts:
#	themes-default/slim/views/addShows_newShow.mako
#	themes-default/slim/views/editShow.mako
#	themes-default/slim/views/inc_addShowOptions.mako
#	themes-default/slim/views/layouts/main.mako
#	themes-default/slim/views/manage_massEdit.mako
#	themes-default/slim/views/vue-components/quality-chooser.mako
#	themes/dark/templates/addShows_newShow.mako
#	themes/dark/templates/editShow.mako
#	themes/dark/templates/inc_addShowOptions.mako
#	themes/dark/templates/layouts/main.mako
#	themes/dark/templates/manage_massEdit.mako
#	themes/dark/templates/vue-components/quality-chooser.mako
#	themes/light/templates/addShows_newShow.mako
#	themes/light/templates/inc_addShowOptions.mako
#	themes/light/templates/layouts/main.mako
#	themes/light/templates/manage_massEdit.mako
#	themes/light/templates/vue-components/quality-chooser.mako
@@ -228,7 +221,7 @@ const startVue = () => {
<span class="component-title">Preferred Quality</span>
<!-- TODO: replace these with a vue component -->
<span class="component-desc">
<quality-chooser @update="saveQualities"></quality-chooser>
<quality-chooser @update:quality:allowed="series.config.qualities.allowed = $event" @update:quality:preferred="series.config.qualities.preferred = $event"></quality-chooser>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah

@p0psicles p0psicles merged commit 130cfca into develop Apr 22, 2018
@p0psicles p0psicles deleted the feature/convert-editshow-to-vue branch April 22, 2018 09:32
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.

5 participants