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

Add QuillJSONField #36

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

Conversation

gokselcoban
Copy link
Contributor

@gokselcoban gokselcoban commented Feb 9, 2021

LeeHanYeong and others added 13 commits July 14, 2020 10:06
* Fix content damage on validation error
* Fix fixtures format and enable null values
* Fixed non-saving data of dynamically created quills
* Poetry fix
* Setup fix

Co-authored-by: Michał Dyczko <michal.dyczko@poczta.pl>
this allows the db backend to apply any optimizations it might want/need
closes LeeHanYeong#26

Co-authored-by: proxi <51172302+3n-k1@users.noreply.github.com>
* Bump to Quill Version 1.3.7

Bump to Quill Version 1.3.7 - This version of Quill fixes Quill Vuln quilljs/quill#2438

Here is the change commit to fix the vuln in Quill quilljs/quill#2439

The Vuln is described here: https://ossindex.sonatype.org/vuln/d96c07dd-81f9-41f6-b2bd-531143bcaeab

* Adding JS/CSS include instructions from README.md

Resolves issue [LeeHanYeong#33](LeeHanYeong#33)
@gokselcoban gokselcoban marked this pull request as draft February 9, 2021 19:08
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

just a small question about the QuillField() function. also, i definitely think this change needs docs, and preferably some tests to make sure bad code (like mine) doesn't get through again. this is a pretty big change for library consumers, and they might not realize it



def QuillField(*args, **kwargs):
return QuillTextField(*args, **kwargs)
Copy link

Choose a reason for hiding this comment

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

is there a reason this returns a text field instead of a json field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Current users of the package, imports and uses QuillField which is based on text field. They may don't want to change the existing approach. It returns the text field for backward capability.

Copy link

Choose a reason for hiding this comment

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

fair point. i would just add a bit to the readme explaining the different fields so new users know what to use

@ghost
Copy link

ghost commented Feb 9, 2021

thanks a bunch for putting in the work! it definitely looks a lot better, both on the backend and the frontend (you fixed a css bug i couldn't figure out! thanks!)

i just had a few small suggestions for documentation/tests

@gokselcoban gokselcoban marked this pull request as ready for review February 10, 2021 16:56
@ghost
Copy link

ghost commented Feb 11, 2021

everything looks good to me :)

@cdesch
Copy link
Contributor

cdesch commented Feb 17, 2021

@void-witch Could you please take a look at issue 38? It looks like the new code is causing an issue.

@mukundshah
Copy link

while using QuillJSONField, delta is still stringified. what am I missing?

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.

None yet

7 participants