To have an idea on how to fully integrate react-streamfield, please check
this CodeSandbox demo.
For more complex examples, see example/index.story.js and
the corresponding demos
for more complex examples.
More documentation will arrive soon!
You can also check out
wagtail-react-streamfield
to see what an integration of this field looks like!
Internet Explorer 11 support
These JavaScript features are used in react-streamfield that are not supported
natively in Internet Explorer 11:
Element.closest(…)
Array.find(…)
Object.entries(…)
CustomEvent
When using react-streamfield for Internet Explorer 11, you need to include
the polyfills found in the section below, otherwise the package will not work
properly.
Polyfills
React-streamfield uses some JavaScript features only available starting
ECMAScript 2015. Some of these features are not handled by browsers such as
Internet Explorer 11.
To maintain compatibility when using react-streamfield, install and import
these polyfills (a polyfill adds a missing JavaScript browser feature):
Yet to have a chance to test this, but we do use :focus-within in a couple of places in c-sf-block.scss so should consider using JS instead or at least forcing it to be a separate selector so it only drops the :focus-within styles and not the :hover :focus too.
Alright! 🙂 I generally don't have the chance to review a whole package, that was a lot of fun. In the end I tried to be as exhaustive as possible with my comments, but a lot of it is fairly minor / quick to address.
I made the review as a pull request that contains all of the package's code, over at thibaudcolas#1, so it would be easier to relate comments to code. But I also summarised most of the comments below, so they are easier to relate to one another, and prioritise. It's probably easier to first read this list, then the comments in the PR.
I also made a PR to address some of the issues below (packaging, dev tools, and API), over at #5, along with the corresponding changes to Wagtail in a branch that builds upon wagtail/wagtail#4942.
Potential problems
These are the code-level problems I would expect to cause the most pain / actual bugs. They are ordered by how important I think they are to address sooner rather than later.
Do not use elements like aside, article, header. They provide no semantic value for a form / widget like StreamField (thibaudcolas#1 (comment))
AddButton's groupedBlockDefinitions – What happens if key is an empty string? It seems like this is going to override the groups (thibaudcolas#1 (comment))
extractText – Use either textContent or innerText depending on the desired outcome (thibaudcolas#1 (comment))
Add CustomEvent polyfill to Wagtail for IE11 support, or change the code not to need it (thibaudcolas#1 (comment))
Clean up listeners / observers set up in RawHtmlFieldInput componentDidMount, which can cause memory leaks (thibaudcolas#1 (comment))
Small thing – do not render the add panel's group name if it's an empty string (thibaudcolas#1 (comment))
Error handling
Generally I haven't seen much error handling code. I would expect the inner script execution to be the most problematic, since it will be very common for third-party code to break.
<BlocksContainer /> isn't valid HTML. We shouldn't encourage people to write HTML that doesn't pass validation. For a semantic-free token/placeholder, use a noscript tag with a data attribute. (thibaudcolas#1 (comment), thibaudcolas#1 (comment))
getIsMobile – Use window.matchMedia to make sure the JS and CSS breakpoint definitions are in sync (thibaudcolas#1 (comment))
Packaging - build & dependenceies
The general problem here is that the library is compiled as if it was an app, instead of a library, with all of its dependencies bundled instead of resolved by npm on install.
Stop bundling dependencies in the compiled/published code, these should be resolved as part of the dependency management and build of the consumer code (Wagtail’s) (thibaudcolas#1 (comment))
Publish the package as both CommonJS and ES modules, with main and module attributes in package.json (thibaudcolas#1 (comment))
Remove any side effects on import, and mark the package as "sideEffects": false, for Webpack consumers (thibaudcolas#1 (comment))
Expose the compiled source (dist) via npm only, not in version control (thibaudcolas#1 (comment))
Consider using Rollup instead of Webpack to achieve all of the above easily (thibaudcolas#1 (comment))
Consider using sass (official Dart implementation) to get rid of the annoying native code compilation of node-sass. (thibaudcolas#1 (comment))
Consider configuring transform-react-remove-prop-types to wrap proptypes instead of removing them, as for a project like this they can be very useful in dev mode. (thibaudcolas#1 (comment))
Consider replacing @babel/plugin-proposal-decorators and decorators syntax with normal higher-order functions/components. (thibaudcolas#1 (comment))
Documentation
To me this is what would be the most worthy of documentation. The blockDefinitions schema is probably this package’s most important API, and the polyfills are its least obvious requirements.
Document the blockDefinitions schema (can be as simple as adding comments to BlockDefinitionType, and linking to this) (thibaudcolas#1 (comment))
Add Autoprefixer to the CSS build (Wagtail already has it, so this is just about having a similar setup for this package’s dev/demo site, so it can be used for cross-browser testing) (thibaudcolas#1 (comment))
Split src/index.scss into multiple files, ideally following the separation between React components (one styles file per component) (thibaudcolas#1 (comment))
Consider using a JS-added --focus class instead of :focus-within, if the functionality is important enough (thibaudcolas#1 (comment))
Accessibility
I'm sure the current StreamField implementation isn't particularly SR-friendly, so we're not aiming super high, but there are obvious improvements to be made here.
AddButton’s + icon should have a text-only label for screen reader users (thibaudcolas#1 (comment))
All other icons should also have text-only labels for screen reader users (thibaudcolas#1 (comment))
react-redux recently released its v6, which uses the new React context API from React 16.4, and introduces a change in behavior that affects this package:
[...] there is a behavior change around dispatching actions in constructors / componentWillMount. Previously, dispatching in a parent component's constructor would cause its children to immediately use the updated state as they mounted, because each component read from the store individually. In version 6, all components read the same current store state value from context, which means the tree will be consistent and not have "tearing". This is an improvement overall, but there may be applications that relied on the existing behavior.
The consequence is that BlocksContainer will fail to render because it does not expect to have access to an uninitialised state. It's generally not recommended to have side-effects in the constructor anyway, moving this init to componentDidMount would make the problem even more obvious.
I can see a few solutions:
Move the initializeStreamField logic out of StreamField, to be done when the store is created
Move initializeStreamField call to componentDidMount, and do not render the BlockContainer until initialisation is over.
Anyway, it's not necessary to upgrade to v6 now. I also have two concerns with the upgrade:
react-beautiful-dnd also uses Redux and react-redux v5, but doesn't declare them as peerDependencies. Using different versions from it would mean they get bundled twice for end users, which I would rather avoid. From memory Redux is fairly small in size, but react-redux is a good 20kb.
Wagtail uses v5, and switching to v6 will require updating Wagtail's code. Shouldn't be a big deal, but it will take a bit of time.
Finally on the react-redux front, I'm surprised that all/most of the components in the package are connected. I would expect the performance to be better if some of the connections were removed, as they clearly duplicate their computation (but use PureComponent or React.memo to still have the same rendering performance)
We are extending the react-streamfield on https://github.com/stamkracht/react-streamfield. Already did some minor updates that gives us a bit more control through the blockDefinitions. We are manipulating the blockDefinition through get_definition so we didn't need todo any Python changes yet.
I've seen the RFC and a lot of it is stuff we can also use and I wonder if we can help you achieving those features? We have a lot of in-house experience with both Python and JS. It would be a shame if we do the same stuff but slightly different, so maybe you can give your thoughts on this.
would be better named --close, as it defines the style for a close button which appears when the panel is open.
Fixing this would mean that we can avoid the back-to-front logic of {% if state == 'open' %}c-sf-add-button--closed{% endif %} in wagtail/wagtail#5476 :-)