-
Notifications
You must be signed in to change notification settings - Fork 7
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
Wagtail 2.0 Compatibility #22
Conversation
DRF used to transparently import `django-filter` but it doesn't anymore in 3.7 (which is required by Wagtail 2.0), see encode/django-rest-framework#5273. As a side effect, close #19.
67dd35c
to
b00e198
Compare
Upgrading other dependencies accordingly so they are compatible with the new React version. Removing test dependencies as they aren't yet used (so it make the upgrade easier).
6435dd5
to
7b0ab54
Compare
It would be too much efforts to support sources for Draftail 0.17 (as of Wagtail 2.0) and Draftail 0.7 (as of WagtailDraftail 0.7), therefore taking the highest of the two. WagtailDraftail will have to receive an update to keep compatibility with this package which is a pain but at the same time, it's pretty unlikely that WagtailDraftail will be used alongside with Wagtail 2.0 now that Draftail is integreated within Wagtail.
Wagtail 2.0 ships with Draftail 0.17 which is a rather big jump from Draftail 0.7 which WagtailDraftail shipped and for which the various sources were built. ab03eed is an attempt at updating the sources to the Draftail 0.17 API. It renames the props and callback accordingly and fix the initial errors, however the modal is completely frozen once open (you can't interact with it nor close it). I imagine it's not getting focus correctly but I can't get to the bottom of it at the moment. FWIW, the chooser works fine when used as a panel or block. Once the above issue is resolved, it should be possible to complete the implementation of the |
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.
👏 nice to see this happening. I mostly looked at the front-end, there are two main issues:
- As you note the modal does not work. This is just because when a source is active, the editor switches to read-only mode which uses
pointer-events: none;
(see springload/draftail@f9d0b23#diff-84eefd2734f75d81b5ec9c33379e9e3bR11). Solutions below. - It would've been obvious if the modal had worked – the models added in the editor do not get displayed correctly. Some parts of their code treat those entities as block-level, while others as inline.
For the modals, there are two potential solutions:
- Quick and dirty – use
pointer-events: auto;
on the modal somewhere around.admin-modal { position: fixed; display: flex; align-items: center; justify-content: center; z-index: 1300; overflow-x: hidden; overflow-y: hidden; } - The proper way - use a portal to render the modal outside of the DOM hierarchy of the editor. It's advised to render modals outside of their calling context so their styles don't get polluted by those of wherever they happen to be triggered from. For example, at the moment the modal's search field gets styles from Wagtail's text field CSS 😅
For model definitions, most likely you will want to use RichUtils.toggleLink
to apply the model entities over text rather than AtomicBlockUtils.insertAtomicBlock
. All of the other code is consistent with this. Otherwise there needs to be some configuration of whether the model is block-level or inline, and then its storage & editor rendering needs to be changed accordingly (e.g. use block
component instead of decorator
when registering the plugin, etc).
A good example could be StockSource
in the official tutorial: http://docs.wagtail.io/en/latest/advanced_topics/customisation/extending_draftail.html
Finally, there are a few more things added in Wagtail 2.2 that this could leverage:
- The ability to declare media (JS and CSS) directly on rich text features so they are loaded when needed – see http://docs.wagtail.io/en/latest/advanced_topics/customisation/extending_draftail.html?highlight=media
- The new
Portal
client-side component, which would make the modal's behaviour nicer http://docs.wagtail.io/en/latest/advanced_topics/customisation/admin_templates.html?highlight=portal#extending-client-side-components
cd wagtailmodelchoosers/ | ||
virtualenv .venv | ||
source ./.venv/bin/activate | ||
pip install -e .[testing,docs] -U |
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.
Should be replaced by make init
, which takes care of the migrations too.
```sh | ||
make help # See what commands are available. | ||
|
||
# TODO: Complete commands |
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.
Could be as simple as using the output of make help
:
make help # See what commands are available.
make init # Install dependencies and initialise for development.
make clean-pyc # Remove Python file artifacts.
make start # Starts the development server.
make lint # Lint the project.
make test # Test the project.
make test-coverage # Run the tests while generating test coverage data.
make test-ci # Continuous integration test suite.
make update-test-fixture # Update test fixture from the db.
make dist # Compile the JS and CSS for release.
make publish # Publishes a new version to pypi.
|
||
const GenericModelEntity = (props) => { | ||
console.log(props); | ||
return React.createElement( |
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.
If you're not using the version of React
directly from Wagtail 2.0 at window.React
, then this should be written with JSX.
Ideally Webpack should be configured so import React from 'react';
gets converted to load React from window.React
automagically.
@@ -1,6 +1,6 @@ | |||
import PropTypes from 'prop-types'; | |||
import React from 'react'; | |||
import { DraftUtils } from 'draftail'; | |||
import { AtomicBlockUtils } from 'draft-js'; |
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.
Could Draft.js be loaded from the version built into Wagtail 2.0+ (window.DraftJS
?) instead of having it installed & bundled separately?
const nextState = AtomicBlockUtils.insertAtomicBlock( | ||
editorState, | ||
contentStateWithEntity.getLastCreatedEntityKey(), | ||
' ', |
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.
Atomic blocks are meant for block-level embeds, while I think most models this chooser is for would be considered inline. What you want is probably closer to https://github.com/springload/draftail/blob/9378913945f05395912a61f4b551e993c735c89f/examples/sources/LinkSource.js#L41-L51:
const contentStateWithEntity = contentState.createEntity(
entityType.type,
entityMutability,
data,
);
const entityKey = contentStateWithEntity.getLastCreatedEntityKey();
const nextState = RichUtils.toggleLink(
editorState,
editorState.getSelection(),
entityKey,
);
If it's indeed meant to be blocks, then the mutability selection above should be removed because atomic blocks cannot be mutable (since they don't use text).
const nextState = AtomicBlockUtils.insertAtomicBlock( | ||
editorState, | ||
contentStateWithEntity.getLastCreatedEntityKey(), | ||
' ', |
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.
Same comment as ModelSource
.
klassname = '{}EntityElementHandler'.format(entity_name.title().replace('_', '')) | ||
|
||
class BaseEntityElementHandler(InlineEntityElementHandler): | ||
mutability = 'IMMUTABLE' |
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.
Should this support both mutable and immutable entities like the client-side code does? I might be missing something.
js_dependencies = ( | ||
'wagtailadmin/js/draftail.js', | ||
'wagtailmodelchoosers/wagtailmodelchoosers.js', | ||
'wagtailmodelchoosers/polyfills.js', |
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.
It feels a bit funny to see the polyfills loaded after the rest of the code. They mostly target window.fetch
– I think this could easily be replaced by jQuery's XMLHttpRequest wrappers, which would have the advantage of supporting cancelation (since this is used for autocomplete fields, it's quite valuable).
@loicteixeira This branch is currently working with |
I'm surprised the modal issue mentioned above is gone but I don't have to investigate it further so I'll take your word for it (otherwise, Thibaud outlines some solutions above). In any case, I do not have the bandwidth to finish this so if Springload has a need for this, you should confirm that everything is indeed working, complete/merge this PR and release a new version. Good luck. |
|
This has been sitting here for way too long. I'll let someone at Springload pick this up if they need to. |
Close #23.