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

vueify start of showheader #5087

Merged
merged 23 commits into from
Sep 8, 2018
Merged

vueify start of showheader #5087

merged 23 commits into from
Sep 8, 2018

Conversation

OmgImAlexis
Copy link
Collaborator

@OmgImAlexis OmgImAlexis commented Aug 30, 2018

This converts a majority of the showheader.mako to Vue.

The two props are used to render the component dynamically without needing to worry about the query params for example when testing or when the "show selector" changes.

@OmgImAlexis OmgImAlexis added In progress Needs review Needs testing Requires testing to make sure it's working as intended Frontend labels Aug 30, 2018
@OmgImAlexis OmgImAlexis added this to the 0.2.9 milestone Aug 30, 2018
@OmgImAlexis OmgImAlexis requested a review from sharkykh August 30, 2018 03:41
Signed-off-by: Alexis Tyler <[email protected]>
@OmgImAlexis OmgImAlexis force-pushed the feature/vueify-showheader branch from 96fc2fc to 10f1e44 Compare August 30, 2018 03:46
@OmgImAlexis

This comment has been minimized.

@sharkykh

This comment has been minimized.

@OmgImAlexis OmgImAlexis mentioned this pull request Aug 30, 2018
const reg = /collapseSeason-(\d+)/g;
const result = reg.exec(this.id);
$('#showseason-' + result[1]).text('Show Episodes');
$('#season-' + result[1] + '-cols').addClass('shadow');
});
$('.collapse.toggle').on('show.bs.collapse', function() {
document.addEventListener('show.bs.collapse', function() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a good chance this is broken.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest:

document.querySelectorAll('.collapse.toggle').forEach(element => {
    element.addEventListener('hide.bs.collapse', function() {
        // on hide
    });
    element.addEventListener('show.bs.collapse', function() {
        // on show
    });
));

And to test this you need to disable Show all seasons in General Config -> Interface.

@sharkykh

This comment has been minimized.

@OmgImAlexis

This comment has been minimized.

Copy link
Contributor

@sharkykh sharkykh left a comment

Choose a reason for hiding this comment

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

I'm not done, but you can start with these...

% endif
<template v-if="config.subtitles.enabled">
<tr><td class="showLegend">Subtitles: </td><td><img :src="'images/' + (show.config.subtitlesEnabled ? 'yes' : 'no') + '16.png'" :alt="show.config.subtitlesEnabled ? 'Y' : 'N'" width="16" height="16" /></td></tr>
</template>
<tr><td class="showLegend">Season Folders: </td><td><img src="images/${("no16.png", "yes16.png")[bool(show.season_folders or app.NAMING_FORCE_FOLDERS)]}" alt="${("N", "Y")[bool(show.season_folders or app.NAMING_FORCE_FOLDERS)]}" width="16" height="16" /></td></tr>
Copy link
Contributor

Choose a reason for hiding this comment

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

<tr><td class="showLegend">Season Folders: </td><td><img :src="'images/' + ((show.config.seasonFolders || config.namingForceFolders) ? 'yes' : 'no') + '16.png'" :alt="(show.config.seasonFolders || config.namingForceFolders) ? 'Y' : 'N'" width="16" height="16" /></td></tr>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't remember where namingForceFolders was.

Copy link
Contributor

Choose a reason for hiding this comment

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

I generally just do a search for app.VAR_NAME in the codebase and look for medusa/server/api/v2/config.py or open up that file and look for the variable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Btw an || doesn't need () around it in a ternary statement.

((show.config.seasonFolders || config.namingForceFolders) ? 'yes' : 'no')

vs

(show.config.seasonFolders || config.namingForceFolders ? 'yes' : 'no')

@@ -152,7 +136,7 @@
<div class="row">
<!-- Show Summary -->
<div id="summary" class="col-md-12">
<div id="show-summary" class="${'summaryFanArt' if app.FANART_BACKGROUND else ''} col-lg-9 col-md-8 col-sm-8 col-xs-12">
<div id="show-summary" :class="[{ summaryFanArt: config.fanArtBackground }, 'col-lg-9', 'col-md-8', 'col-sm-8', 'col-xs-12']">
Copy link
Contributor

Choose a reason for hiding this comment

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

config.fanartBackground
Lowercase a.

% if xem_numbering or xem_absolute_numbering:
<app-link href="http://thexem.de/search?q=${show.name | h}" title="http://thexem.de/search?q-${show.name | h}">
<img alt="[xem]" height="16" width="16" src="images/xem.png" style="margin-top: -1px; vertical-align:middle;"/>
</app-link>
% endif
<app-link href="https://fanart.tv/series/${show.indexerid}" title="https://fanart.tv/series/${show.name | h}"><img alt="[fanart.tv]" height="16" width="16" src="images/fanart.tv.png" class="fanart"/></app-link>
## @TODO: Is this meant to use show.title for title but show.id[show.indexer] for href?
<app-link :href="'https://fanart.tv/series/' + show.id[show.indexer]" :title="'https://fanart.tv/series/' + show.title"><img alt="[fanart.tv]" height="16" width="16" src="images/fanart.tv.png" class="fanart"/></app-link>
Copy link
Contributor

@sharkykh sharkykh Aug 30, 2018

Choose a reason for hiding this comment

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

  1. show.id[show.indexer] for both.
  2. I think fanart.tv only supports TVDB ids so it should only be shown for shows that have a TVDB id
  3. The id could possibly be in show.externals for non-TVDB shows, but that's not available through the API right now...

<app-link href="${indexerApi(show.indexer).config['show_url']}${show.indexerid}" title="${indexerApi(show.indexer).config["show_url"] + str(show.indexerid)}">
<img alt="${indexerApi(show.indexer).name | h}" height="16" width="16" src="images/${indexerApi(show.indexer).config["icon"]}" style="margin-top: -1px; vertical-align:middle;"/>
</app-link>
## @TODO: What is this?
Copy link
Contributor

Choose a reason for hiding this comment

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

The link to the show on the indexer website.
image

@@ -10,64 +10,47 @@
<%namespace file="/inc_defs.mako" import="renderQualityPill"/>

<div class="row">
<div id="showtitle" class="col-lg-12" data-showname="${show.name | h}">
<div id="showtitle" class="col-lg-12" :data-showname="show.title">
Copy link
Contributor

@sharkykh sharkykh Aug 30, 2018

Choose a reason for hiding this comment

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

Can't find any usage for data-showname (or data('showname')) in the code.
Can probably be removed.

Scratch that:
https://github.com/pymedusa/Medusa/blob/feature/vueify-showheader/themes-default/slim/views/vue-components/sub-menu.mako#L78
Maybe we can use something else.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall I skip this for now? I could add a @TODO comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, go ahead with a @TODO.

<div>
<h1 class="title" data-indexer-name="${show.indexer_name}" data-series-id="${show.indexerid}" id="scene_exception_${show.indexerid}">
<app-link href="home/displayShow?indexername=${show.indexer_name}&seriesid=${show.indexerid}" class="snatchTitle">${show.name | h}</app-link>
<h1 class="title" :data-indexer-name="show.indexer" :data-series-id="show.id[show.indexer]" :id="'scene_exception_' + show.id[show.indexer]">
Copy link
Contributor

Choose a reason for hiding this comment

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

:data-indexer-name="show.indexer"
:data-series-id="show.id[show.indexer]"
:id="'scene_exception_' + show.id[show.indexer]"

As far as I can tell, you can remove these three props.
The first two are used in scene-exceptions-tooltip.js, however the file isn't imported anywhere ATM.
That file is used to put a 'qtip' tooltip on the show title, that lists the show's scene names.
If we want that, we should convert that to Vue in a separate PR.

Copy link
Collaborator Author

@OmgImAlexis OmgImAlexis Aug 30, 2018

Choose a reason for hiding this comment

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

If we want that, we should convert that to Vue in a separate PR.

Sounds good.

% if not app.DISPLAY_SHOW_SPECIALS and season_special:
<% lastSeason = season_results.pop(-1) %>
% endif
<template v-if="$route.name !== 'snatchSelection' && show.seasons && show.seasons.length >= 1">
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless I'm reading this wrong,
you can do without the <template> wrapper, and just put the v-if on the next element?

% endif
% endfor
% endif
<template v-if="show.seasons && show.seasons.length >= 13">
Copy link
Contributor

Choose a reason for hiding this comment

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

You already checked for show.seasons above, so you know it's not undefined / null.
Plus, same note as above, no need for the <template> wrapper, put it on the <select> element,
or better yet, the <span> element it's currently in.
As long as you don't use <element v-for="..." v-if="..."> you should be good to go.

Copy link
Contributor

Choose a reason for hiding this comment

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

I could be misreading that, but to me it looks like you changed the logic here:

len(season_results) > 14

is not the same as

show.seasons.length >= 13
14 > 14 : false
14 >= 13 : true

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So it should be >= 15. Will change.

</option>
</select>
</template>
<template v-else-if="show.seasons && show.seasons.length >= 1">
Copy link
Contributor

Choose a reason for hiding this comment

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

You already checked for show.seasons above, so you know it's not undefined / null.
If you use my previous suggestion to use <span>, then you can also drop the <template> wrapper, and just put the if on the <span> element.

Even if you have to add another empty <span v-else></span> element, I still feel like it's much more readable.

const reg = /collapseSeason-(\d+)/g;
const result = reg.exec(this.id);
$('#showseason-' + result[1]).text('Show Episodes');
$('#season-' + result[1] + '-cols').addClass('shadow');
});
$('.collapse.toggle').on('show.bs.collapse', function() {
document.addEventListener('show.bs.collapse', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest:

document.querySelectorAll('.collapse.toggle').forEach(element => {
    element.addEventListener('hide.bs.collapse', function() {
        // on hide
    });
    element.addEventListener('show.bs.collapse', function() {
        // on show
    });
));

And to test this you need to disable Show all seasons in General Config -> Interface.

>
<span :style="{ width: (Number(show.rating.imdb.rating) * 12) + '%' }"></span>
</span>
<template v-if="!show.rating.imdb">
Copy link
Contributor

Choose a reason for hiding this comment

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

<template v-if="!show.rating.imdb">

is not the same as

% if not show.imdb_id:

I think you thought it was

% if not show.imdb_info:

It should probably be something like:

<template v-if="!show.id.imdb">

});
// Selects all visible episode checkboxes
document.addEventListener('click', event => {
if (event.target && event.target.className.includes('clearAll')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That class is incorrect, should be seriesCheck.

});
[...document.getElementsByClassName('seasonCheck')].filter(isVisible).forEach(element => {
element.checked = true;
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use this instead of the two loops, but it's probably going to be replaced later..

[...document.querySelectorAll('.epCheck, .seasonCheck')].filter(isVisible).forEach(element => {
    element.checked = true;
});

% endif
% endfor
% endif
<template v-if="show.seasons && show.seasons.length >= 13">
Copy link
Contributor

Choose a reason for hiding this comment

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

I could be misreading that, but to me it looks like you changed the logic here:

len(season_results) > 14

is not the same as

show.seasons.length >= 13
14 > 14 : false
14 >= 13 : true

@OmgImAlexis
Copy link
Collaborator Author

@sharkykh I ran git merge origin/develop and then it said I had to update the vendors so I re-ran yarn dev. Hope that's right.

@sharkykh
Copy link
Contributor

sharkykh commented Sep 6, 2018

Yes that is correct.
You had to do that because I edited snatch-selection.vue on #5107, and it had a conflict.

yarn install ; yarn dev after a merge or rebase is my go-to action.

@@ -24,7 +24,10 @@
<div v-if="$route.name === 'snatchSelection'" id="show-specials-and-seasons" class="pull-right">
<span class="h2footer display-specials">
Manual search for:<br>
<app-link href="home/displayShow?indexername=${show.indexer_name}&seriesid=${show.series_id}" class="snatchTitle">{{ show.title }}</app-link> / Season {{ season }}<template v-if="episode"> Episode {{ episode }}</template>
<app-link
href="home/displayShow?indexername=${show.indexer_name}&seriesid=${show.series_id}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The href is still using Python values, change to:
:href="'home/displayShow?indexername=' + show.indexer + '&seriesid=' + show.id[show.indexer]"

<app-link
href="home/displayShow?indexername=${show.indexer_name}&seriesid=${show.series_id}"
class="snatchTitle"
>{{ show.title }}</app-link> / Season {{ season }}<template v-if="episode && !$route.query.manual_search_type"> Episode {{ episode }}</template>
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably should move $route.query.manual_search_type into a computed property, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In another PR. Should be fine for now.

Signed-off-by: Alexis Tyler <[email protected]>
- IMDB tooltip
- Remove unused IMDB stars jQuery function
- reflowLayout after `show` changes
- Update load/resize event handlers
@OmgImAlexis
Copy link
Collaborator Author

@sharkykh would you mind a last review and then merge?

@OmgImAlexis
Copy link
Collaborator Author

Should we add show_message to show.message in the show python dict?

@sharkykh
Copy link
Contributor

sharkykh commented Sep 8, 2018

Still a small bug. I think I may have created it.

2018-09-08 16:45:00 WARNING  TORNADO :: [6354111] 404 GET /images/null (127.0.0.1) 8.00ms

Looking into it now.

Should we add show_message to show.message in the show python dict?

👍 (in another PR since you want to merge this)

@OmgImAlexis

This comment has been minimized.

@sharkykh
Copy link
Contributor

sharkykh commented Sep 8, 2018

@OmgImAlexis No :) You're confusing the two PRs you have.
Already found and fixed it, it was my indexer icon code. Forgot a condition.
Will push in a minute..

@OmgImAlexis OmgImAlexis merged commit 4ff8f90 into develop Sep 8, 2018
@OmgImAlexis OmgImAlexis deleted the feature/vueify-showheader branch September 8, 2018 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concluded Frontend Needs review Needs testing Requires testing to make sure it's working as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants