Skip to content

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jan 8, 2019

peek 2019-01-08 13-18

Replaces #102 #100

Requires nextcloud/server#13407

On every page load 1 request 593B

  • Load about.js (593B) to handle the about menu

On first page load / on click About in menu 1 request 97KB

  • Load firstrunwizard.js (97KB)

Before 3 requests 39.7 KB

  • Load firstrunwizard.js (4.1KB)
  • Load jquery.colorbox.js (29KB)
  • Load firstrunwizard.js (6.6KB)

ToDo

  • cleanup css rules
  • maybe just use plain XMLHttpRequest instead of axios (would probably save another 10kb)
  • CSS vars polyfill isn't working for IE11, so we probably need to keep the separate css file

@juliusknorr juliusknorr added this to the Nextcloud 16 milestone Jan 8, 2019
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@jancborchardt Here is the adjusted version with a proper primary button:

peek 2019-01-09 14-14

Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Love the animation! 💥 Also, much better with the blue primary-styled next button. :)

Design-wise 👍, did not review the code.

@rullzer
Copy link
Member

rullzer commented Jan 13, 2019

@juliushaertl awesome!

Does it still launch the first time on a new user?
I'm actually fine with the axios overhead. It will not be loaded all the time now anyway. Which is more than fine but up to you of course.

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
</template>
<style lang="scss">

/* Page styling needs to be unscoped, since we load it separately from the server */
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me. I do not see how the async loading affects this.. am I missing something or should it just work?

Copy link
Member Author

Choose a reason for hiding this comment

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

The scoping will not apply since the html from the individual pages is not part of the vue bundle.

As in this screenshot, if the styles are scoped the .page class will get scoped, but it isn't added to the html element, since that one is inserted as plain html using v-html.

image

Copy link
Member

Choose a reason for hiding this comment

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

If you really want to scope it, you can still manually scope the full css like we do in the components :)

Signed-off-by: Julius Härtl <jus@bitgrid.net>
@jancborchardt
Copy link
Member

Is anything special required to build/make this or only the core pull request?

Also, could you also test it on dark theme? I just did some fixes – at https://github.com/nextcloud/firstrunwizard/tree/darktheme-fixes cause I forgot this branch exists. ;)

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

See comments and the following list: :)

  • Missing makefile

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

@jancborchardt I also fixed the dark mode issues in here. 😉

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Amazing work!! 🚢

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Tested and works like a charm with current master. Lets get this in!

@rullzer rullzer merged commit 34b1a99 into master Jan 17, 2019
@rullzer rullzer deleted the vue branch January 17, 2019 19:50
@jancborchardt
Copy link
Member

@juliushaertl nice work! :) Could you add the command to build it to the readme?

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.

5 participants