Skip to content

1.0.0 #31

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Jun 7, 2018
Merged

1.0.0 #31

merged 11 commits into from
Jun 7, 2018

Conversation

f
Copy link
Owner

@f f commented Jun 6, 2018 •
edited
Loading

v1.0.0

  • A complete rewrite, more extensible.
  • Readable and better code.
  • Update to Webpack 4
  • Remove built-in loaders. Maybe we can create another repository including custom spinners.
  • Remove width and height props.
  • Strict props.
  • isLoading supports matcher now.
  • Rename registerComponents to registerComponent
  • Added accessorName option to change $vueLoading key.
  • Removed createActionHelpers, use mapLoadingActions or wrapLoading instead.

@f f requested a review from sky-code June 6, 2018 12:42
@f
Copy link
Owner Author

f commented Jun 6, 2018

@sky-code I'm in rewriting progress. Can you please review?

@f
Copy link
Owner Author

f commented Jun 6, 2018

Diff view is untrackable, please pull and view code on your editor :)

@f f changed the title WIP: 1.0.0 1.0.0 Jun 6, 2018
Koc
Koc reviewed Jun 6, 2018
View reviewed changes
.travis.yml

node_js:
- 5
- 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it compatible with Node 8?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Possibly yes. Not tested.

Koc
Koc reviewed Jun 6, 2018
View reviewed changes
README.md Outdated

```html
<template>
<progress-bar v-if="$vueLoading.isLoading(/creating.*/)">Creating something...</progress-bar>
Copy link
Contributor

Choose a reason for hiding this comment

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

Dot requires escaping. WDYT about using wildcards? It simpler and not requires escaping. We can outsource it to library like https://github.com/sindresorhus/matcher

@Koc
Copy link
Contributor

Koc commented Jun 6, 2018 •
edited
Loading

Code is much better readable now. Can you fix #28 also before 1.0, please?

And WDYT about repository rename? To vue-loading (and remove paragraph about misleading about Vuex).

@f
Copy link
Owner Author

f commented Jun 6, 2018

I think #28 is fixed with vuexModuleName option. Now no named property is hardcoded, everything is configurable.

I'm thinking about renaming the project but vue-loading is already registered. Maybe we can find another fancy name.

@Koc
Copy link
Contributor

Koc commented Jun 6, 2018

I think #28 is fixed with vuexModuleName option. Now no named property is hardcoded, everything is configurable.

So should I use different vuexModuleName for each Vue instance? If yes - it doesn't sove issue, because we need share same module between different instances.

Koc
Koc reviewed Jun 6, 2018
View reviewed changes
src/vue-loading.js Outdated
throw new Error('[vuex-loading] Vuex not initialized.');
}
this.store = store;
store.registerModule(vuexModuleName, vuexStore);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is possible to check here - if module with same name already registered and if yes - skip registration? Can you add test case for multiple Vue instances on same page?

Copy link
Owner Author

Choose a reason for hiding this comment

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

This makes sense. I'm going to.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Vuex doesn't give me the info about if the module is registered, so I unregister it first.

@Koc Koc mentioned this pull request Jun 6, 2018
Closed
@f
Copy link
Owner Author

f commented Jun 6, 2018

Added some useful directives. ec2db9d

@f f merged commit 0869a6e into master Jun 7, 2018
@f f deleted the 1.0.0 branch June 7, 2018 07:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sky-code sky-code Awaiting requested review from sky-code

1 more reviewer

@Koc Koc Koc left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@f @Koc