Skip to content

Initial version of Squire integration - #2390

Merged
tomholub merged 13 commits into
masterfrom
issue-1861-add-squire
Dec 19, 2019
Merged

tomholub merged 13 commits into
masterfrom
issue-1861-add-squire

Conversation

@limonte

@limonte limonte commented Dec 17, 2019

Copy link
Copy Markdown

Closes #1861

@tomholub the PR is mostly ready. If I implemented something totally wrong, let me know. I'll polish the remaining details (e.g. height of the reply iframe overflows) tomorrow.

@limonte
limonte force-pushed the issue-1861-add-squire branch from 7a1e732 to aa688b8 Compare December 17, 2019 22:08

@tomholub tomholub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Very good. Some notes / suggestions below.

Because I didn't want to make 30 comments: since we only use squire in compose.htm, I would only include it there. Only libraries that have a potential to be used anywhere (like sweetalert) would go into every htm file.

Also, I'll have to look into how we process images (either when sending, or when receiving) because the image doesn't get rendered on the other end (doesn't properly pass sanitization).

image

Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-input.ts Outdated
Comment thread extension/chrome/elements/composer/composer-size.ts Outdated
Comment thread extension/lib/squire-raw.js
Comment thread extension/types/squire.d.ts
Comment thread flowcrypt-browser.code-workspace
@limonte

limonte commented Dec 18, 2019

Copy link
Copy Markdown
Author

Because I didn't want to make 30 comments: since we only use squire in compose.htm, I would only include it there. Only libraries that have a potential to be used anywhere (like sweetalert) would go into every htm file.

Done, thanks!

@tomholub

Copy link
Copy Markdown
Collaborator

For the failing test, it looks like it just wants a .trim() somewhere:

image

I would not mind to .trim() the value right in the test itself, because it doesn't look like the newline will hurt anything, but will leave it up to you to investigate.

@limonte

limonte commented Dec 18, 2019

Copy link
Copy Markdown
Author

@tomholub while working on the height overflowing, I noticed quite a nasty issue: fastmail/Squire#374

Let's see if it'll be fixed on the Squire side. If not, then we'll need to come up with some workarounds.

@tomholub

Copy link
Copy Markdown
Collaborator

@tomholub while working on the height overflowing, I noticed quite a nasty issue: neilj/Squire#374

Let's see if it'll be fixed on the Squire side. If not, then we'll need to come up with some workarounds.

Out of curiosity, try this fork: https://github.com/ProtonMail/Squire

@tomholub

Copy link
Copy Markdown
Collaborator

@tomholub while working on the height overflowing, I noticed quite a nasty issue: neilj/Squire#374
Let's see if it'll be fixed on the Squire side. If not, then we'll need to come up with some workarounds.

Out of curiosity, try this fork: https://github.com/ProtonMail/Squire

Sorry, no need - I looked at the code diff and it doesn't look relevant.

@limonte
limonte force-pushed the issue-1861-add-squire branch from 85243da to 82135f8 Compare December 18, 2019 23:18
@limonte

limonte commented Dec 19, 2019

Copy link
Copy Markdown
Author

For the failing test, it looks like it just wants a .trim() somewhere:

image

I would not mind to .trim() the value right in the test itself, because it doesn't look like the newline will hurt anything, but will leave it up to you to investigate.

Done, thanks 👍

@limonte
limonte marked this pull request as ready for review December 19, 2019 01:19
@limonte

limonte commented Dec 19, 2019

Copy link
Copy Markdown
Author

The PR is ready for review. Please let me know your decision on how to process inline images. Also, I'd appreciate if you help me with #2390 (comment)

@tomholub

Copy link
Copy Markdown
Collaborator

I fixed the images. I suspect there may still be problems in special cases like images in quoted content, images in draft and so on. We'll evaluate and test these one by one, and add automated tests too.

@tomholub
tomholub merged commit a07b185 into master Dec 19, 2019
@limonte
limonte deleted the issue-1861-add-squire branch December 19, 2019 12:48
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.

integrate html editor into compose box

2 participants