-
-
Notifications
You must be signed in to change notification settings - Fork 42
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 replaceMode onto User Options #73
Conversation
…er class when mode is false
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.
This looks good to me. Were you able to test it in your app? Also, could you add a test for this to https://github.com/single-spa/single-spa-vue/blob/main/src/single-spa-vue.test.js?
Hey, @joeldenning, sure. I'll start the tests and trying it out now, just wanted to be sure that this was what we are looking for. Is there any other specific scenarios I should be worried about with this new condition? Or that's all good? |
We should verify that the following code would result in the following html: singleSpaVue({
el: '#vue-app',
replaceMode: true,
// rest of options
}) <template>
<div id="vue-app">
<button>Hi</button>
</div>
</template> <html>
<body>
<div id="vue-app">
<button>Hi</button>
</div>
</body>
</html> |
@joeldenning, done. Should have TDD'ed here, but oh well. |
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.
Thanks
I documented this in single-spa/single-spa.js.org#443 and released it in https://github.com/single-spa/single-spa-vue/releases/tag/v2.3.0 |
This PR is related to #64 discussion, and it is a pretty naive implementation of what I understand would suffice to allow users to mount their applications onto the specified DOM element, and avoid the concatenation of
.single-spa-container
class.