Skip to content

Conversation

@DavidRouyer
Copy link
Contributor

@DavidRouyer DavidRouyer commented Aug 22, 2018

Related issues

Issue #1540
Continuation of #1613
Streamined bodybuilder version & fixed bodybuilder types

Short description and why it's useful

Screenshots of visual changes before/after (if there are any)

Screenshot of passed e2e tests (if you are using our standard setup as a backend)

(run yarn tests:e2e and paste the results. If you are not using our standard backend setup or demo.vuestorefront.io you can ommit this step)

Contribution and curently important rules acceptance

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

@DavidRouyer won’t it be better than having rootStore.state.config.cart.synchronize_totals (violates Demeter Principle) to have just single line after toot store is imported:
config = rootStore.state.config

?

@DavidRouyer
Copy link
Contributor Author

I'd prefer people to use the rootState and get used to it, and the store is only used to read the state, but I can change it if you want!

@pkarw
Copy link
Collaborator

pkarw commented Aug 24, 2018

@DavidRouyer I don't like such a long statements (i mean rootStore.state.config.<section>.<property>)

Please change it if You could and let me know when it's ready to be merged in?

@DavidRouyer
Copy link
Contributor Author

@pkarw I've addressed your feedback, can't do better in multistore.js because prepareStoreView already contains a config parameter

@DavidRouyer
Copy link
Contributor Author

Hm, I can't do that because I'm passing it by value and not by reference :/

@pkarw
Copy link
Collaborator

pkarw commented Aug 26, 2018

That’s fine. Can I merge it in?

@DavidRouyer
Copy link
Contributor Author

I've made a lot of clean up, now the config is loaded once in app.ts. I can't use the const config = rootStore.state.config after the imports because the object is passed by value, and the initial state of the config is {}. You can use it inside the functions, but at this point it's just 💄. Please merge and change it case by case if you want to.

Copy link
Collaborator

@pkarw pkarw left a comment

Choose a reason for hiding this comment

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

OK,
I got it.
We can leave it as it is

@pkarw pkarw merged commit 0d1871f into vuestorefront:develop Aug 28, 2018
@DavidRouyer DavidRouyer deleted the no-globals branch August 28, 2018 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants