Skip to content

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

Merged
merged 25 commits into from
Feb 5, 2020
Merged

Add Vue integration docs #283

merged 25 commits into from
Feb 5, 2020

Conversation

jacekbogdanski
Copy link
Member

@jacekbogdanski jacekbogdanski commented Jan 10, 2020

Important

Before merging this PR we need to update package.json file with published ckeditor4-vue npm package. Currently, it uses a dev repository with the post-install script for development purposes. So, instead of:

"ckeditor4-vue": "https://github.com/ckeditor/ckeditor4-vue#develop",

"postinstall": "cd node_modules/ckeditor4-vue && npm i && npm run build",

we should have:

"ckeditor4-vue": "0.0.1"

Closes ckeditor/ckeditor4-vue#7.
Closes ckeditor/ckeditor4#3774.

Copy link
Contributor

@f1ames f1ames left a 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:

image

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:

image

and the Integrations on the main page too:

image


Something strange with the sample menu styling going on (Chrome 79 on Linux Mint 19.2, FF seems to be working fine) 🤔

image

Seems to be happening for smaller resolution:

image


Can we also polish the styling of this example:

image

I know w e have the same for React but it looks quite cramped.

@jacekbogdanski jacekbogdanski self-assigned this Jan 23, 2020
@jacekbogdanski
Copy link
Member Author

jacekbogdanski commented Jan 23, 2020

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?

Also do we need:
docs/sdk/examples/assets/vue/main.js
docs/sdk/examples/assets/vue/main.js.map

We don't. I forgot to update .gitignore for these files.

And when switching to NPM package we will get rid of scripts/integrations/vue/webpack.config.js right?

Not really. It's building sample code, not integration. You can see the same approach with React and Angular sample apps.

Something strange with the sample menu styling going on (Chrome 79 on Linux Mint 19.2, FF seems to be working fine) thinking

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

Can we also polish the styling of this example:

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.

Copy link
Contributor

@f1ames f1ames left a 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 🤔 🤞

@f1ames f1ames force-pushed the t/ckeditor4-vue/7 branch from 5a2663f to de9a562 Compare January 28, 2020 08:12
@f1ames
Copy link
Contributor

f1ames commented Jan 28, 2020

Rebased onto latest stable.

@f1ames
Copy link
Contributor

f1ames commented Jan 28, 2020

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).

Copy link
Contributor

@AnnaTomanek AnnaTomanek left a 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.

@f1ames f1ames force-pushed the t/ckeditor4-vue/7 branch from 4758532 to 3017497 Compare February 5, 2020 08:43
@f1ames
Copy link
Contributor

f1ames commented Feb 5, 2020

Rebased onto latest stable.

@f1ames f1ames merged commit 1f6a420 into stable Feb 5, 2020
@CKEditorBot CKEditorBot deleted the t/ckeditor4-vue/7 branch February 5, 2020 08:47
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.

3 participants