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

Feature/fully vueify notifications #4913

Merged
merged 68 commits into from
Oct 29, 2018
Merged

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Aug 18, 2018

  • 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:

  • create component config-toggle-slider
  • create component config-textbox
  • kodi
  • plex media server
  • plex home
  • emby
  • nmj
  • nmjv2
  • synology
  • synology notifier
  • pyTivo
  • growl
  • prowl
  • libnotify
  • pushover
  • boxcar2
  • pushalot
  • pushbullet (jquery removed & tested)
  • freemobile
  • telegram (jquery not removed & tested)
  • twitter (jquery removed & tested)
  • trakt (jquery not removed)
  • email ((jquery not fully removed)
  • slack (jquery not removed)
  • save config
  • Test if all the jquery still works. As allot of $('id') needs to be replaced with $('id').find('input')
  • update api-description
  • fix build (need to update the test fixtures)
  • add snotify

…lly-vueify-notifications

# Conflicts:
#	themes-default/slim/views/layouts/main.mako
#	themes/dark/assets/js/app.js
#	themes/dark/assets/js/index.js
#	themes/dark/assets/js/vendors.js
#	themes/light/assets/js/app.js
#	themes/light/assets/js/index.js
#	themes/light/assets/js/vendors.js
#	themes/light/templates/layouts/main.mako
Make it available for usage from mako files.
* Rename Config-checkbox to config-toggle-slider.
* Rebuild themes.
@p0psicles p0psicles added this to the 0.2.9 milestone Aug 18, 2018
@OmgImAlexis OmgImAlexis self-requested a review August 19, 2018 05:24
@p0psicles p0psicles mentioned this pull request Aug 20, 2018
@OmgImAlexis OmgImAlexis mentioned this pull request Aug 21, 2018
@p0psicles p0psicles self-assigned this Aug 21, 2018
@p0psicles
Copy link
Contributor Author

@OmgImAlexis can you check the overal setup now? As next i'll add all the notifiers to apiv2 and the page. As that's allot of work, i'd like to limit the refactoring to as little as possible.

…lly-vueify-notifications

# Conflicts:
#	themes-default/slim/src/index.js
#	themes/dark/assets/js/index.js
#	themes/dark/assets/js/vendors.js
#	themes/light/assets/js/index.js
#	themes/light/assets/js/vendors.js
@sharkykh sharkykh added the Changelog Requires a changelog entry label Aug 23, 2018
@sharkykh
Copy link
Contributor

@p0psicles Move it to 0.2.10?

@p0psicles
Copy link
Contributor Author

If current notifications page is acceptable. But think thats in master already anyway.
Not sure, havent run master in long time

@OmgImAlexis OmgImAlexis modified the milestones: 0.2.9, 0.2.10 Aug 29, 2018
@sharkykh
Copy link
Contributor

If current notifications page is acceptable. But think thats in master already anyway.
Not sure, havent run master in long time

Have you found a bug in the current notifications page?

</label>
<div class="col-sm-10 content">
<input type="text" name="plex_server_token" id="plex_server_token" v-model="notifiers.plex.server.token" @change="save()" @update="notifiers.plex.server.token = $event" class="form-control input-sm max-input350"/>
<!-- Can't use the config-textbox component, because of the complex descriptions -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can be fixed by using a slot in the config-textbox try this or something similar.

diff --git a/themes-default/slim/src/components/config-textbox.vue b/themes-default/slim/src/components/config-textbox.vue
index cceb95db3..dd7bd569b 100644
--- a/themes-default/slim/src/components/config-textbox.vue
+++ b/themes-default/slim/src/components/config-textbox.vue
@@ -5,10 +5,10 @@
                 <label :for="id" class="col-sm-2 control-label">
                     <span>{{ label }}</span>
                 </label>
-                <div class="col-sm-10 content">
+                <slot class="col-sm-10 content">
                     <input :type="type" v-bind="{id, name: id}" v-model="localValue" :class="inputClass" :placeholder="placeholder"/>
                     <p v-for="(explanation, index) in explanations" :key="index">{{ explanation }}</p>
-                </div>
+                </slot>
             </div>
         </div>
     </div>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a new component </> which is really bare. And just sets up the divs and styling. This makes use of slots.

This component can be used to create a standard bootstrapped label/content form-group.
<slot> can be used to parse your own content.
Use v-model in stead of :value.
Added <slot></slot>.
* Utilized the new config-template component.
* Utilized config-textbox slots.
…lly-vueify-notifications

# Conflicts:
#	themes-default/slim/src/components/index.js
#	themes/dark/assets/js/medusa-runtime.js
#	themes/light/assets/js/medusa-runtime.js
@ghost
Copy link

ghost commented Oct 28, 2018

DeepCode analyzed this pull request.
There is 1 new info report.

Click to see more details.

Moved config-template.vue component to helpers.
@@ -1,13 +1,14 @@
<template>
<div id="config-textbox-content">
<div id="config-textbox">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind if this is used more than once on the page you'll have two items with the same ID. I'd go with classes instead.

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

@@ -92,15 +92,18 @@ export default {
this.indexCounter += 1;
},
addNewItem() {
const { newItem, editItems } = this;
if (this.newItem === '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

When comparing use the const value, not this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

little bit down i'm assiging to newItem:
this.newItem = '';
Should I still use the const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This this is also correct?:

deleteItem(item) {
            const { editItems } = this;
            this.editItems = editItems.filter(e => e !== item);
            this.$refs.newItemInput.focus();
            this.$emit('change', 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.

I could go even further and destructure also $refs and $emit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this would be "correct":

deleteItem(item) {
            const { $refs, $emit, editItems } = this;
            this.editItems = editItems.filter(e => e !== item);
            $refs.newItemInput.focus();
            $emit('change', editItems);
        },

placeholder: String,
selectClass: {
type: String,
default: 'select-show form-control input-sm-custom'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should selectClass only be select-show?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry don't understand the question

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this will replace the classes won't form-control and input-sm-custom be needed to keep it looking like a form? I would have though just select-show needs to be replaced.

Copy link
Contributor Author

@p0psicles p0psicles Oct 29, 2018

Choose a reason for hiding this comment

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

This just allows it to be customized. Maybe you don't want to use the bootstrap classes at all. In my case, i'm using show-selector in config-notifications. And there I don't want the select-show class, which is mainly used for the switching shows component. And style it for different viewports.
image

image

OmgImAlexis
OmgImAlexis previously approved these changes Oct 29, 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.

Frontend looks good to me.

@p0psicles
Copy link
Contributor Author

@medariox okay?

…lly-vueify-notifications

# Conflicts:
#	themes/dark/assets/js/index.js.map
#	themes/dark/assets/js/vendors.js.map
#	themes/light/assets/js/index.js.map
#	themes/light/assets/js/vendors.js.map
@medariox medariox changed the title [WIP] Feature/fully vueify notifications Feature/fully vueify notifications Oct 29, 2018
@medariox medariox merged commit 98aeff7 into develop Oct 29, 2018
@sharkykh sharkykh deleted the feature/fully-vueify-notifications branch November 4, 2018 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants