-
Notifications
You must be signed in to change notification settings - Fork 144
Add Vue integration docs #283
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
Conversation
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.
Looks quite nice 👍
Some polishing needed and few questions:
I had some npm i
issue due to package-lock.json
file:
so I'm not sure if we should push it? Also do we need:
docs/sdk/examples/assets/vue/main.js
docs/sdk/examples/assets/vue/main.js.map
shouldn't those be added during docs build process? Because theses are just Vue integration dist files?
And when switching to NPM package we will get rid of scripts/integrations/vue/webpack.config.js
right?
The installation section should be updated:
and the Integrations on the main page too:
Something strange with the sample menu styling going on (Chrome 79 on Linux Mint 19.2, FF seems to be working fine) 🤔
Seems to be happening for smaller resolution:
Can we also polish the styling of this example:
I know w e have the same for React but it looks quite cramped.
I can't reproduce it. Maybe it's not related to PR changes?
We don't. I forgot to update .gitignore for these files.
Not really. It's building sample code, not integration. You can see the same approach with React and Angular sample apps.
Seems like Vue treats HTML standards very seriously and doesn't trim empty spaces between list tags like it's done in HTML specification: vuejs/vue#7701
Honestly, it looks fine for me, but I don't mind updating it. However, maybe it makes more sense to extract it as a separate issue, so we can unify examples between Vue, React and Angular? Otherwise, we will end up with 3 different samples sharing a lot of similarities instead of a standardized approach. |
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.
LGTM, it went pretty smooth 👍 No we need a final review from @AnnaTomanek 🙏
Honestly, it looks fine for me, but I don't mind updating it. However, maybe it makes more sense to extract it as a separate issue, so we can unify examples between Vue, React and Angular? Otherwise, we will end up with 3 different samples sharing a lot of similarities instead of a standardized approach.
Makes sense, can you extract it @jacekbogdanski?
I had some npm i issue due to package-lock.json file:
I can't reproduce it. Maybe it's not related to PR changes?
Seems to be working fine now 🤔 🤞
5a2663f
to
de9a562
Compare
Rebased onto latest |
WARNING: Before merging this PR we need to update package.json file with published ckeditor4-vue npm package (see #283 (comment)).☝️ Means it could be merged only after NPM package is published and should be done after Vue package release (by @f1ames or @jacekbogdanski). |
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 added some minor fixes but overall, great job @jacekbogdanski 👏 ! All fine by me.
Co-Authored-By: Krzysztof Krztoń <[email protected]>
4758532
to
3017497
Compare
Rebased onto latest |
Important
Before merging this PR we need to update
package.json
file with publishedckeditor4-vue
npm package. Currently, it uses a dev repository with the post-install script for development purposes. So, instead of:ckeditor4-docs/package.json
Line 26 in 76aeec5
ckeditor4-docs/package.json
Line 68 in 76aeec5
we should have:
Closes ckeditor/ckeditor4-vue#7.
Closes ckeditor/ckeditor4#3774.