feat: remove nuxt-loading div from DOM when not used #3891

pimlie · 2018-09-15T11:23:31Z

feat: split nuxt-loading.vue in js/css files
feat: use css classes instead of style props
feat: add loading.css property to fully ignore the default css

Types of changes

New feature (non-breaking change which adds functionality)

Description

Remove the nuxt-loading div from the DOM when its not used and fully use css-classes instead of style properties. Also add a loading.css boolean property so you can fully ignore the default nuxt css.

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly. (PR: feat: continuous loading description docs#924)
I have added tests to cover my changes (if not applicable, please state why)
All new and existing tests passed.

Will add a docs PR if this gets approved.

feat: split nuxt-loading.vie in js/css files feat: add css classes instead of style props feat: add loader.css property

galvez

@pimlie take a look at the failing test -- LGTM otherwise

pimlie · 2018-09-15T12:00:46Z

@galvez Yeah sorry, I wasnt sure whether I caused that issue and I didnt want to touch things I did not specifically change. But looking at it now I understand why ;)

galvez · 2018-09-15T12:08:41Z

@pimlie Can you also add a test that determines it's not being included when not used?

pimlie · 2018-09-15T12:18:34Z

Do you mean the div or the css? Or both ;)

pimlie · 2018-09-15T14:54:29Z

I 'fixed' the tests on circleci by adding a Utils.waitFor(100) at the start of the new test. It works now, but this could possibly trigger intermittent errors when the circleci servers are busy. Its not a perfect solution but I guess the tests are quicker triggered then puppeteer / chrome can handle all DOM events and I cant think of another solution atm to wait for the DOM events to finish.

Let me know if anyone has a better solution, maybe otherwise just put this test in a separate test file?

pi0 · 2018-09-15T15:58:00Z

lib/app/components/nuxt-loading.js

-
-<script>
-import Vue from 'vue'
+<% if (loading && loading.css) { %>import "./nuxt-loading.css"<% } %>


Please keep this file to be SFC. We can still conditionally do the same for the <style>...</style> part. Splitting just makes maintenance harder.

Will revert this back :)

pi0 · 2018-09-15T15:59:15Z

lib/app/App.js

@@ -1,5 +1,5 @@
 import Vue from 'vue'
-<% if (loading) { %>import NuxtLoading from '<%= (typeof loading === "string" ? loading : "./components/nuxt-loading.vue") %>'<% } %>
+<% if (loading) { %>import NuxtLoading from '<%= (typeof loading === "string" ? loading : "./components/nuxt-loading.js") %>'<% } %>


If we hardcode .js here, the user can't override nuxt-loading component. Currently, he/she can do this by creating app/components/nuxt-loading component or use a module to change it.

Would you be ok with omitting the file extension in that case? I am not fully sure if that will always work, but in my experience omitting the file extensions resolves in either .vue or .js.

pi0 · 2018-09-15T16:00:33Z

lib/app/components/nuxt-loading.js


 export default {
  name: 'nuxt-loading',
+  render(h) {


Render function instead of<template> seems a good idea for this 👍

Atinux · 2018-09-18T14:29:53Z

I agree with @pi0 requested changes, looking forward to this PR :)

add nuxt-loading component tests with vue-server-renderer revert flaky e2e progressbar test add nuxt.options.loading.throttle option with defualt value 200

pimlie · 2018-09-18T21:18:23Z

This PR is ready for review again. These last changes introduce one breaking change: before if you paused loading with $loading.pause() you could / needed to restart loading again by calling $loading.start(). This has now changed to $loading.resume(). If you call start() the loading will always start from 0 again. Hope you agree with me thats better as otherwise you also have the throttle timeout after resuming every pause :)

If you wish I can work on this tomorrow, but the e2e tests are probably not as they should be. E.g. the basic browser /noloading test actually errors out to a nuxt-error page which is not detected at all. We should add a listener for errors to fully make sure there are none.
Btw, the /noloading page errors out because this.$nuxt.$loading is not yet available when mounted() is called. It is only available after a nextTick(). Also think the start/finish calls are reversed. @galvez could you maybe confirm / have a look at this?

~~Not sure why the circleci:audit test failed, looks like a temporary CI issue?~~ Yes it was, guess it was good this time I noticed a bug in one of the tests

add pause/resume test

codecov-io · 2018-09-18T21:45:51Z

Codecov Report

Merging #3891 into dev will decrease coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #3891      +/-   ##
==========================================
- Coverage   97.64%   97.56%   -0.09%     
==========================================
  Files          18       18              
  Lines        1231     1231              
  Branches      335      335              
==========================================
- Hits         1202     1201       -1     
- Misses         28       29       +1     
  Partials        1        1

Impacted Files	Coverage Δ
lib/common/nuxt.config.js	`100% <ø> (ø)`	⬆️
lib/core/nuxt.js	`94.16% <0%> (-0.84%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65432e6...0dd374b. Read the comment docs.

codecov-io · 2018-09-18T21:46:03Z

Codecov Report

Merging #3891 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #3891   +/-   ##
=======================================
  Coverage   87.83%   87.83%           
=======================================
  Files          43       43           
  Lines        1701     1701           
  Branches      435      435           
=======================================
  Hits         1494     1494           
  Misses        180      180           
  Partials       27       27

Impacted Files	Coverage Δ
packages/config/src/config/index.js	`0% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e49764c...1c9f0ff. Read the comment docs.

Atinux · 2018-09-19T09:31:03Z

test/e2e/basic.browser.test.js

-    const { hook } = await page.nuxt.navigate('/noloading')
-    const loading = await page.nuxt.loadingData()
+    const { hook } = await page.nuxt.navigate('/noloading', false)
+    await Utils.waitFor(nuxt.options.loading.throttle + 100)


Could we maybe move this right into page.nuxt.navigate @pimlie ?

Yes, or we could add a call to page.waitForFunction instead. But as mentioned I actually think the e2e tests need some more refactoring anyway as there doesnt seem to be enough protection against unexpected exceptions at the moment. Maybe we can change this with that refactoring?

feat: new config option 'continuous' If true the progress bar keeps changing also after duration time has passed. It switches from ltr growing to rtl shrinking, then ltr growing again with ltr: false fix: remove Math.random() for percent increase, it caused choppy behaviour and more importantly when loading took exactly duration time there was a 100% chance the progress bar was not yet at 100% fix: add safeguards so percent can never be <0 or >100 fix: calculate _cut correctly for duration < 1000 fix: remove automatically finishing when >95% is reached. This behaviour was counter-intuitive, when duration time had almost passed duration the progress bar always finished, regardless whether it was truly finished or not. Now the progress bar stays at 100% until loading really finishes (with continuous = false) feat: add nuxt-progess-notransition class to disable transitions when we are switching from grow to shrink

pimlie · 2018-09-24T19:59:20Z

Please see the message of commit e7bddc9 for all the details.

i just like to do things more difficult :s

fix: add yarn.lock after vue-jest install

old

pi0 · 2018-10-27T16:37:06Z

packages/common/src/nuxt.config.js

@@ -194,7 +194,9 @@ export default {
    height: '2px',
    throttle: 200,
    duration: 5000,
-    rtl: false
+    continuous: false,


Docs should be updated to add continuous too

pimlie · 2018-10-28T16:56:27Z

I have added a PR for the missing docs (you can also check that PR for a gif of the continuous loading)

pi0

LGTM

packages/app/template/components/nuxt-loading.vue

lock · 2018-11-28T17:42:03Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

feat: remove nuxt-loading div from DOM when not used

39c079f

feat: split nuxt-loading.vie in js/css files feat: add css classes instead of style props feat: add loader.css property

galvez suggested changes Sep 15, 2018

View reviewed changes

fix: update new extract-css hash

f31323a

pimlie added 3 commits September 15, 2018 14:59

add loading/progressbar test

aa213a0

lint fixes

39f02dc

fix test

86e6403

pi0 previously requested changes Sep 15, 2018

View reviewed changes

pimlie added 4 commits September 18, 2018 17:31

revert back to SFC

a522392

rewrite extract css test to remove [hash] dep

c1e7c78

Merge branch 'dev' into feat-render-loading

773814c

clean-up nuxt-loading even more

1f3a1d2

add nuxt-loading component tests with vue-server-renderer revert flaky e2e progressbar test add nuxt.options.loading.throttle option with defualt value 200

fix throttle test

0dd374b

add pause/resume test

fix: Use vue ext

5509537

Atinux reviewed Sep 19, 2018

View reviewed changes

galvez assigned pimlie Sep 20, 2018

galvez previously approved these changes Sep 20, 2018

View reviewed changes

galvez added post-2.0 and removed post-2.0 labels Sep 20, 2018

pimlie mentioned this pull request Sep 23, 2018

loading: add throttle option to skip progress for fast loads #3886

Merged

2 tasks

ishitatsuyuki mentioned this pull request Sep 24, 2018

[1/2] expose loading.throttle as an option #3953

Merged

pimlie added 4 commits September 24, 2018 21:15

Merge branch 'dev' into feat-render-loading

d07ff18

Merge branch 'feat-render-loading' of into feat-render-loading

1629eaa

Merge branch 'dev' into feat-render-loading

7dfcb22

pimlie added 4 commits September 25, 2018 08:00

revert fix for _cut which wasnt broken at all

28d4b65

i just like to do things more difficult :s

Merge branch 'dev' into feat-render-loading

571d126

Merge branch 'dev' into feat-render-loading

6ae6626

fix: change waitFor import

a47745f

fix: add yarn.lock after vue-jest install

pimlie dismissed galvez’s stale review via a47745f October 27, 2018 15:54

fix: lint and import in e2e

3593f71

pi0 previously approved these changes Oct 27, 2018

View reviewed changes

fix: explicitly set left style, false doesnt remove it once set

8242fe5

pimlie dismissed pi0’s stale review via 8242fe5 October 28, 2018 16:33

pimlie added 3 commits October 28, 2018 17:38

Merge branch 'dev' into feat-render-loading

44048a8

fix: remove nuxt.config from common

f354d50

fix: remove unnecessary decimal test

3eadb1c

pimlie mentioned this pull request Oct 28, 2018

feat: continuous loading description nuxt/docs#924

Merged

fix: wait longer for azure

1c9f0ff

pi0 approved these changes Oct 29, 2018

View reviewed changes

manniL reviewed Oct 29, 2018

View reviewed changes

packages/app/template/components/nuxt-loading.vue Show resolved Hide resolved

manniL approved these changes Oct 29, 2018

View reviewed changes

aldarund approved these changes Oct 29, 2018

View reviewed changes

pi0 merged commit 72961ac into nuxt:dev Oct 29, 2018

pimlie deleted the feat-render-loading branch October 29, 2018 17:10

lock bot locked as resolved and limited conversation to collaborators Nov 28, 2018

danielroe added the 2.x label Jan 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: remove nuxt-loading div from DOM when not used #3891

feat: remove nuxt-loading div from DOM when not used #3891

pimlie commented Sep 15, 2018 •

edited by manniL

galvez left a comment

pimlie commented Sep 15, 2018

galvez commented Sep 15, 2018

pimlie commented Sep 15, 2018 •

edited

pimlie commented Sep 15, 2018

pi0 Sep 15, 2018

pimlie Sep 15, 2018

pi0 Sep 15, 2018

pimlie Sep 15, 2018

pi0 Sep 15, 2018

Atinux commented Sep 18, 2018

pimlie commented Sep 18, 2018 •

edited

codecov-io commented Sep 18, 2018

codecov-io commented Sep 18, 2018 •

edited

Atinux Sep 19, 2018

pimlie Sep 19, 2018

pimlie commented Sep 24, 2018 •

edited

pi0 Oct 27, 2018

pimlie commented Oct 28, 2018

pi0 left a comment •

edited

lock bot commented Nov 28, 2018

feat: remove nuxt-loading div from DOM when not used #3891

feat: remove nuxt-loading div from DOM when not used #3891

Conversation

pimlie commented Sep 15, 2018 • edited by manniL

Types of changes

Description

Checklist:

galvez left a comment

Choose a reason for hiding this comment

pimlie commented Sep 15, 2018

galvez commented Sep 15, 2018

pimlie commented Sep 15, 2018 • edited

pimlie commented Sep 15, 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

Atinux commented Sep 18, 2018

pimlie commented Sep 18, 2018 • edited

codecov-io commented Sep 18, 2018

Codecov Report

codecov-io commented Sep 18, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pimlie commented Sep 24, 2018 • edited

Choose a reason for hiding this comment

pimlie commented Oct 28, 2018

pi0 left a comment • edited

Choose a reason for hiding this comment

lock bot commented Nov 28, 2018

pimlie commented Sep 15, 2018 •

edited by manniL

pimlie commented Sep 15, 2018 •

edited

pimlie commented Sep 18, 2018 •

edited

codecov-io commented Sep 18, 2018 •

edited

pimlie commented Sep 24, 2018 •

edited

pi0 left a comment •

edited