-
Notifications
You must be signed in to change notification settings - Fork 80
Improve typings for events and update project demo #244
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
Pull Request Test Coverage Report for Build 459
💛 - Coveralls |
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 noticed a few problems while checking the PR.
-
If I understand correctly,
yarn dev
andyarn build
commands in the demo app will build the component from the root path'../'
(not from npm). If so, thedevelop
command is no longer needed in the mainpackage.json
. On the other hand, we build the production version for npm using webpack. Now we don't have a sample app in this repository that would verify if what webpack produces still works. For example, the demo app built with vite might work, but not with webpack. -
In the demo app, the
yarn dev
command works fine. Theyarn build
command failed:Do you know this? What could be the reason?
-
After migrating to yarn, the demo app still contains
package-lock.json
and there are also other references to npm, e.g. in theREADME.md
.
Vite will not build the component from source, but will use code compiled by Webpack. The path You can test this by removing the
Fixed.
Updated. |
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.
As I am not in the process, I do not understand why we have two samples:
sample/
demo/
I can guess that the first is just a sample using the component and some CKEditor 5 build, while the second is a demo application (more realistic scenario). It would be good to describe both in README and explain which should be checked before releasing a new version. Maybe we do not need the sample/
directory anymore. Instead, we should develop example applications that could be checked using E2E tests on CI.
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 PR removes the demo in the sample
folder, so we only have the one in the demo
folder.
I updated the README to include information on how to run the new demo before releasing a new version.
I also added a new CI script that will build the demo project.
Any more sophisticated tests will require more work and are probably beyond the scope of this PR. Any thoughts?
b76acb5
to
3c7cffa
Compare
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 🎉
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.
Suggested merge commit message (convention)
Feature: Improve typings for events emitted by the Vue component.
Internal: Add Vue linter and apply suggestions.
Internal: Update demo project to better reflect real-world usage. Closes ckeditor/ckeditor5#13545.
MAJOR BREAKING CHANGE: Integration now requires Editor version 37 or later.