Skip to content
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

Upgrade rich-text and rich-text-presence example to Quill 2.0.2 #683

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

horner
Copy link

@horner horner commented Oct 6, 2024

This pull request includes several updates to the examples/rich-text-presence project, specifically to support Quill 2.0.2 and modernizing the build process, updating dependencies, and improving the user interface.

Screenshot 2024-10-06 at 5 57 10 AM

Watch: Demo

The most important changes are summarized below:

Modernization of Build Process:

  • Switch from Browserify to Webpack: Updated the build script in package.json to use Webpack for module bundling, replacing Browserify. Added webpack and webpack-cli to devDependencies, and included a new webpack.config.js file for configuration. (examples/rich-text-presence/package.json, examples/rich-text-presence/webpack.config.js) [1] [2] [3]

Dependency Updates:

  • Upgrade Quill: Updated the quill dependency from version ^1.3.7 to ^2.0.2 in package.json. (examples/rich-text-presence/package.json)

Codebase Improvements:

  • Migration to ES6 Imports: Replaced CommonJS require statements with ES6 import statements in client.js for better compatibility with modern JavaScript standards. (examples/rich-text-presence/client.js)

User Interface Enhancements:

  • Change Quill Theme: Modified the Quill editor initialization to use the snow theme instead of bubble and added a toolbar module for enhanced editing capabilities. Correspondingly, added the quill.snow.css stylesheet in index.html. (examples/rich-text-presence/client.js, examples/rich-text-presence/static/index.html) [1] [2]

@horner horner changed the title Upgrade rich-text-presence example to Quill 2.0.2 Upgrade rich-text and rich-text-presence example to Quill 2.0.2 Oct 6, 2024
Copy link
Collaborator

@alecgibson alecgibson left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Some notes mostly on keeping things minimalist

"express": "^4.18.2",
"quill": "^1.3.7",
"quill": "^2.0.2",
"quill-cursors": "^4.0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not used in this example

@@ -15,14 +15,20 @@
"license": "MIT",
"dependencies": {
"@teamwork/websocket-json-stream": "^2.0.0",
"bson-objectid": "^2.0.4",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not used in this example

Comment on lines 28 to 30
"@babel/core": "^7.25.7",
"@babel/preset-env": "^7.25.7",
"babel-loader": "^9.2.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need these? I think if you remove babel from the Webpack config, everything still runs (in a modern browser). If possible, let's keep these examples as bare-bones as possible (it's not within the scope of this library to inform consumers how to set up their build chain).

Similar comment applies for rich-text-presence.

Comment on lines 9 to 22
module: {
rules: [
{
test: /\.js$/,
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
presets: ['@babel/preset-env']
}
}
}
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

As noted above, I don't think this is needed. Let's remove to keep as minimalist as possible.

theme: 'bubble',
modules: {cursors: true}
var quill = new Quill('#editor', {
theme: 'snow',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason in particular for changing the theme?

Copy link
Author

Choose a reason for hiding this comment

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

The theme has a toolbar. It is easier to test behavior cursor and the toolbar interactions with it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🤷🏼‍♂️ I guess it doesn't matter which theme we use, but if you need to do any particularly in-depth testing with cursors and toolbar interaction, that should really be a concern of quill-cursors.

@horner
Copy link
Author

horner commented Oct 23, 2024

@alecgibson Thanks for the idea to KISS it. Babel removed.

theme: 'bubble',
modules: {cursors: true}
var quill = new Quill('#editor', {
theme: 'snow',
Copy link
Collaborator

Choose a reason for hiding this comment

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

🤷🏼‍♂️ I guess it doesn't matter which theme we use, but if you need to do any particularly in-depth testing with cursors and toolbar interaction, that should really be a concern of quill-cursors.

Comment on lines +9 to +16
module: {
rules: [
{
test: /\.js$/,
exclude: /node_modules/,
}
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

As I said before, this isn't needed.

Please remove.

Comment on lines +17 to +19
resolve: {
extensions: ['.js']
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

You actually also don't need this.

Comment on lines 9 to 25
module: {
rules: [
{
test: /\.js$/,
exclude: /node_modules/,
use: {
loader: 'babel-loader',
options: {
presets: ['@babel/preset-env']
}
}
}
]
},
resolve: {
extensions: ['.js']
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above: unneeded; please remove

@@ -3,6 +3,7 @@
<title>ShareDB Rich Text</title>
<meta name="viewport" content="width=device-width, initial-scale=1, maximum-scale=1, user-scalable=0"/>
<link href="quill.bubble.css" rel="stylesheet">
<link href="quill.snow.css" rel="stylesheet">
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're changing theme here, please remove bubble.css

Comment on lines 30 to 32
"@babel/core": "^7.25.7",
"@babel/preset-env": "^7.25.7",
"babel-loader": "^9.2.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unneeded.

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