Skip to content
This repository has been archived by the owner on Dec 11, 2021. It is now read-only.

Code review #4

Open
51 of 74 tasks
thibaudcolas opened this issue Jan 21, 2019 · 3 comments
Open
51 of 74 tasks

Code review #4

thibaudcolas opened this issue Jan 21, 2019 · 3 comments
Labels
enhancement New feature or request

Comments

@thibaudcolas
Copy link
Contributor

thibaudcolas commented Jan 21, 2019 •
edited by BertrandBordage

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.

Performance

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.

Minor ones

Minor but still sources of concern

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.

Bonus points

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.

Development & demo env

Styles

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.

react-redux

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.

This is the problematic code:

https://github.com/noripyt/react-streamfield/blob/9b720dd715140f5c931560e75431ea472600f9be/src/StreamField.js#L95-L107

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)

This would also make it easier to write tests for those components, which I would really like to see.

Smaller JS / React things
This was referenced Jan 21, 2019
Open
Merged
@BertrandBordage BertrandBordage mentioned this issue Feb 22, 2019
Closed
@jonnyscholes jonnyscholes mentioned this issue Mar 7, 2019
Merged
@jonnyscholes
Copy link
Contributor

This PR #6 should resolve the following:

  • ✅ Overly generic class names across all of the UI (e.g. icon, field, required) – these should use BEM, and-or be namespaced. Everything is now using BEM and is namespaced with c-sf inline with our new ITCSS inspired structure. Where sf stands for streamfield.
  • ✅ Do not use elements like aside, article, header. They provide no semantic value for a form / widget like StreamField
  • ✅ Use BEM notation for element selectors, e.g. .add-block-button instead of button.add
  • ✅ Replace bare element names by class names, e.g. .block-header instead of header
  • ✅ Do not use h3 for BlockHeader's icon There does seem to be a case where a heading level is reasonable so I've removed it only when it's the icon only variant.
  • ⚠️ Split src/index.scss into multiple files, ideally following the separation between React components (one styles file per component) I've done this for the most part - but haven't quite made a separate component that exactly matches each react file. In some cases multiple SCSS components just made sense to separate them because they are very visually different components - eg .c-sf-add-button vs .c-sf-add-panel. In others there were technical reasons - .c-sf-block in particular has a lot of children that are inter-dependant. Pulling them out into multiple files made it more complex to parse. I left some notes about this in the SCSS for that component for things we could do to improve this if need be but I don't think they should block release.
  • ⚠️ Use BEM notation for state selectors, e.g. .children-container--dragging instead of .children-container.is-dragging As discussed in Slack, I've gone with is-/has- state classes in most cases. Both methods seem to be in use across the Wagtail code base and so I went with the least effort case until we review our CSS/JS styleguide. Happy to be overridden here :)

@thibaudcolas would be good to get you to confirm and provide your thoughts :)

@thibaudcolas
Copy link
Contributor Author

Sweet, I'll try to have a look this weekend!

@BertrandBordage
Copy link
Contributor

Thank you very much to both of you for these contributions!

@thibaudcolas I improved a lot of things as well after @jonnyscholes’ contribution.
I updated the checkboxes above. As you can see, there is mainly one important thing remaining that I plan to do after we release the first integration to Wagtail: the error handling. I will go through the same incremental updates you did for Draftail.

I leave this review open for me to fix the remaining unticked checkboxes. But for now, this is already more than good to be in Wagtail :)

@BertrandBordage BertrandBordage pinned this issue Mar 11, 2019
@BertrandBordage BertrandBordage mentioned this issue Mar 11, 2019
Closed
@BertrandBordage BertrandBordage added the enhancement New feature or request label Aug 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@thibaudcolas @BertrandBordage @jonnyscholes