-
Notifications
You must be signed in to change notification settings - Fork 17
add-vuejs-gs-with-vuex #414
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
add-vuejs-gs-with-vuex #414
Conversation
Codecov Report
@@ Coverage Diff @@
## save-vuejs-getting-started #414 +/- ##
==========================================================
Coverage 96.3% 96.3%
==========================================================
Files 32 32
Lines 1517 1517
==========================================================
Hits 1461 1461
Misses 56 56 Continue to review full report at Codecov.
|
commit('ADD_MESSAGE', notification.result); | ||
} | ||
}); | ||
commit('SET_ROOMID', roomID); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You set the roomId in the store but you never use it. I think you should unsubscribe this roomId at some point (when the route change for example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effectively, I did this in the other vuejs gettings-started at line 103, and i explained why I set the roomId in comments because in this little app I don't think its better to weighing the router.
Should I change that in both vuejs getting-started ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just worried people would not understand why we do this in the getting started so I think it would be better, yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll do the changes :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still breaks the documentation build for me:
docker run --rm -t -v $PWD/doc/6:/var/app/src/sdk/js/6 -v /var/app/node_modules/@vuepress/node_modules/.cache-loader -p 8080:8080 kuzzleio/documentation:dev
I must admit I don't have a clear solution for the moment to this issue but it definitely needs to be solved before it is merged to the documentation repository.
Maybe a tweak to exclude html
files from the frontmatter plugin would be good enough. I will try that.
[Edit] looks like an exclude
pattern is already applied but does not seem to work. I'll look at it.
[Edit] finally looks the issue was coming from the docker image which did not include the exclude pattern. Seems to work far better now.
- Additionally, the target branch of this PR looks strange to me. Is it the correct one?
- Aren't we missing an
index.md
file in thewith-vuex
directory?
@benoitvidis We made the save branch to remove vuejs getting started of master and allow us to build the doc* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok :)
What does this PR do?
Add vuejs getting-started using vuex
Template and CSS are the same as #405 just move logical part into vuex store
How should this be manually tested?
Other changes
/
Boyscout
/