Vue #95

nickvergessen · 2017-10-20T12:28:58Z

@georgehrke mind having a look at the Vue stuff? I think it was pretty easy and straight forward after reading the docs

Fix #2

Todos:

Migrate JS to Vue.JS
Use webpack to manage the JS
Change .js files to .vue files (HTML not in strings and phpstorm support)
- Reactivate actions and deleting
Add make file to development
Clarify with @MorrisJobke how we can do this for packaging.

nickvergessen · 2018-01-08T09:34:37Z

Rebased on top of the latest changes
@MariusBluem feel free to test it.

MorrisJobke · 2018-01-08T10:34:14Z

@nickvergessen Do we really want it in 13 or wait for 14?

nickvergessen · 2018-01-08T10:39:23Z

I don't care too much, it comes with the nice addition that the icon is now always visible and there is a decent empty content view.

But yeah it's a complete rewrite of the js.

MariusBluem · 2018-01-08T13:06:50Z

@nickvergessen Do we really want it in 13 or wait for 14?

13 please since having the bell always would be ❤️ for the release 😄

With empty content it looks nice, but when you have notfications, the UI is a bit broken (at least on Safari) ...

not possible to close a notification
style is broken:

MorrisJobke · 2018-01-08T13:09:25Z

13 please since having the bell always would be ❤️ for the release 😄

We only have the RC stage left and I would avoid to bring in completely rewritten code that late. :/

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2018-01-22T12:56:32Z

@ChristophWurst can you check the last commit, is that correct?

If so we need to discuss with @MorrisJobke how we do that for shipped apps?
Add a special command to a make file?

ChristophWurst · 2018-01-22T13:05:50Z

js/components/root.js

@@ -7,6 +7,8 @@
 * later. See the COPYING file.
 */

+require('./notification');


Actually, the intended use is that the require call returns the module. The way you're using it webpack will load the module, but the module then just registers some stuff in a global variable which you later access. This is kinda hacky and error-prone.

I don't know if it's easy to migrate here, but get rid of the OCA.XXX = xxx stuff. Don't use those globals and instead, have webpack locate the module and inside the module you export whatever you previously registered as global variable.

Does that make sense? :)

See nextcloud/twofactor_u2f@ae4fe1c#diff-ccd4d3da9150404f0f6690291ba3add9 as an example. The view class is exported because it's the returned value of the module definition.

Worked fine for the components I think.
in app.js the exporting to OCA.Notifications is okay I guess?

in app.js the exporting to OCA.Notifications is okay I guess?

Is OCA.Notifications used elsewhere, outside of this app?

If not, how about making app.js a module like the other components which you then load in a init.js script where you start the app like

define(function(require) { 'use strict'; var $ = require('jquery') var App = require('./app') $(function() { var app = new App() app.initialize() }) })

ReferenceError: define is not defined

😢

is reported for app.js but I dont get why :/

Hmmmm but you don't include the app.js file directly, right? It's bundled with webpack.

Yeah, tested it a bit, when I remove import Vue from 'vue'; it works. should I move that somewhere else, or replace it with a different call?

ChristophWurst · 2018-01-22T14:15:35Z

Yeah, tested it a bit, when I remove import Vue from 'vue'; it works. should I move that somewhere else, or replace it with a different call?

Yes, correct. Just cloned this branch and you're apparently mixing module syntax. The import … syntax is ES6, while define … is commonjs module syntax.

Use var Vue = require('vue'); instead.

nickvergessen · 2018-01-22T15:46:49Z

Use var Vue = require('vue'); instead.

TypeError: e is not a constructor

When I do require('vue'); I get ReferenceError: Vue is not defined

ChristophWurst · 2018-01-22T15:56:34Z

vuejs/vue#5470 (comment) maybe

nickvergessen · 2018-01-22T16:30:11Z

That worked, squashed it all

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2018-01-24T09:01:24Z

@MorrisJobke make package should be ready for testing.
Can you also check that strings are correctly extracted from .vue files?

notifications/js-src/components/notification.vue

Line 108 in 8f0dff0

    
           						OC.Notification.showTemporary(t('notifications', 'Failed to dismiss notification'));

is a new string

ChristophWurst · 2018-01-24T09:18:09Z

js-src/components/action.vue

+	}
+</script>
+
+<style scoped>


I guess you can omit the style tag if it's unused

Yes, tested and done

nickvergessen · 2018-01-24T09:36:17Z

Last two thing I don't understand:

it always generates a js/merged.js.map file.
Since we don't load that with script() I think we don't need to ship it.
Can we also stop generating it? And if so, how?

Currently it seems to always package the dev version of vue, as per console:

You are running Vue in development mode.
Make sure to turn on production mode when deploying for production.
See more tips at https://vuejs.org/guide/deployment.html

Can that be easily adjusted for the package?

Signed-off-by: Joas Schilling <coding@schilljs.com>

ChristophWurst · 2018-01-24T11:20:20Z

it always generates a js/merged.js.map file.
Since we don't load that with script() I think we don't need to ship it.
Can we also stop generating it? And if so, how?

They are very useful for debugging, e.g. when people report errors on the client-side, the browser will otherwise not be able to tell you where the error occurred in your respective source file. Hence, it will just show error on line 254485 -> not helpful at all.
To my understanding the browser will load this automatically and webpack should add a source map comment to your bundled script file: https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map

nickvergessen · 2018-01-24T11:24:14Z

They are very useful for debugging, e.g. when people report errors on the client-side, the browser will otherwise not be able to tell you where the error occurred in your respective source file. Hence, it will just show error on line 254485 -> not helpful at all.
To my understanding the browser will load this automatically and webpack should add a source map comment to your bundled script file: https://developer.mozilla.org/en-US/docs/Tools/Debugger/How_to/Use_a_source_map

Ah okay, so we will keep it and ship it. Fine by me, just didnt know what this is about

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen · 2018-01-24T11:56:26Z

Fixed the vue dev issue, although that should work automatically as per https://vuejs.org/v2/guide/deployment.html
However, it did not, so now I build in a hard coded switch for it.

Ready to review and merge:
@ChristophWurst @MorrisJobke @georgehrke @juliushaertl

ChristophWurst

Code looks good so far, didn't test though

ChristophWurst · 2018-01-26T12:10:19Z

Makefile

+	--exclude=/.drone.yml \
+	--exclude=/node_modules \
+	--exclude=/npm-debug.log \
+	--exclude=/package.json \


if you're using an up-to-date version of npm, it will generate a package-lock.json. You might want to exclude that too.

dont have that on npm 5.6.0

Okay, maybe it's just generated if you use npm install --save xxx or similar https://docs.npmjs.com/files/package-lock.json

Hmm let me add this, so everyone is installing the same stuff?

Exactly, it locks down the exact version number, just like composer does with composer.lock

Oh, and it speeds up npm install as npm doesn't have to build the dependency graph 👍

ChristophWurst · 2018-01-26T12:14:36Z

package.json

+    "vue": "^2.5.13"
+  },
+  "devDependencies": {
+    "babel-core": "^6.26.0",


not sure if you actually need/use babel. Looks like it's all ES5 and thus can be run in all our supported browsers.

ChristophWurst · 2018-01-26T12:15:02Z

package.json

+    "vue-loader": "^13.7.0",
+    "vue-template-compiler": "^2.5.13",
+    "webpack": "^3.6.0",
+    "webpack-dev-server": "^2.11.0"


dev-server is probably not needed and can be removed

Signed-off-by: Joas Schilling <coding@schilljs.com>

ChristophWurst

Looks good to me!

MariusBluem

Tested. Works 👍

nickvergessen added 3. to review enhancement feature: frontend 🖌️ labels Oct 20, 2017

nickvergessen added this to the Nextcloud 13 milestone Oct 20, 2017

nickvergessen requested review from georgehrke and skjnldsv October 20, 2017 12:28

nickvergessen force-pushed the vue branch from 1b29122 to 71ccce0 Compare October 20, 2017 12:29

nickvergessen mentioned this pull request Oct 20, 2017

adieu Angular1, welcome Vue nextcloud/calendar#540

Closed

5 tasks

nickvergessen mentioned this pull request Jan 3, 2018

Don't hide bell-icon when there are no notifications #107

Closed

nickvergessen force-pushed the vue branch from 71ccce0 to b53be62 Compare January 8, 2018 09:33

nickvergessen changed the title ~~Vue + Scss~~ Vue Jan 8, 2018

nickvergessen removed the request for review from skjnldsv January 8, 2018 09:34

MorrisJobke modified the milestones: Nextcloud 13, Nextcloud 14 Jan 18, 2018

nickvergessen mentioned this pull request Jan 18, 2018

Migrate Update notifications to Vue.js nextcloud/server#7933

Merged

Vue

949715b

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the vue branch from b53be62 to 2170692 Compare January 22, 2018 11:47

ChristophWurst reviewed Jan 22, 2018

View reviewed changes

nickvergessen force-pushed the vue branch from 360da88 to 1074aa8 Compare January 22, 2018 16:28

WebPack

7aefbc9

Signed-off-by: Joas Schilling <coding@schilljs.com>

Makefile

57a0387

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the vue branch from 1074aa8 to 57a0387 Compare January 24, 2018 08:21

ChristophWurst reviewed Jan 24, 2018

View reviewed changes

.vue files

b59df84

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the vue branch 2 times, most recently from 3687744 to d1d788a Compare January 24, 2018 10:11

Update README.md with information

007a291

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen force-pushed the vue branch from d1d788a to 007a291 Compare January 24, 2018 10:13

No dev verion in production

57eacd2

Signed-off-by: Joas Schilling <coding@schilljs.com>

nickvergessen requested review from MorrisJobke, ChristophWurst and juliusknorr January 24, 2018 11:56

ChristophWurst reviewed Jan 26, 2018

View reviewed changes

nickvergessen added 2 commits January 26, 2018 14:29

Less dependencies

94d4058

Signed-off-by: Joas Schilling <coding@schilljs.com>

Commit package-lock.json

33f54af

Signed-off-by: Joas Schilling <coding@schilljs.com>

ChristophWurst approved these changes Jan 26, 2018

View reviewed changes

MariusBluem approved these changes Jan 26, 2018

View reviewed changes

ChristophWurst added 4. to release and removed 3. to review labels Jan 26, 2018

MorrisJobke approved these changes Jan 26, 2018

View reviewed changes

MorrisJobke merged commit 43d0608 into master Jan 26, 2018

MorrisJobke deleted the vue branch January 26, 2018 21:51

MariusBluem mentioned this pull request Jun 29, 2018

Add icon without red dot for red theming colors #135

Closed

korelstar mentioned this pull request Dec 4, 2018

Move away from AngularJS, ideally to Vue.js (standard for Nextcloud apps) nextcloud/notes#241

Closed

Vue #95

Vue #95

Conversation

nickvergessen commented Oct 20, 2017 • edited Loading

nickvergessen commented Jan 8, 2018

MorrisJobke commented Jan 8, 2018

nickvergessen commented Jan 8, 2018

MariusBluem commented Jan 8, 2018

MorrisJobke commented Jan 8, 2018

nickvergessen commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Jan 22, 2018

nickvergessen commented Jan 22, 2018

ChristophWurst commented Jan 22, 2018

nickvergessen commented Jan 22, 2018

nickvergessen commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Jan 24, 2018 • edited Loading

ChristophWurst commented Jan 24, 2018

nickvergessen commented Jan 24, 2018

nickvergessen commented Jan 24, 2018

ChristophWurst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst left a comment

Choose a reason for hiding this comment

MariusBluem left a comment

Choose a reason for hiding this comment

nickvergessen commented Oct 20, 2017 •

edited

Loading

nickvergessen commented Jan 24, 2018 •

edited

Loading