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

Code review for upcoming Wagtail integration #1

Open
wants to merge 81 commits into
base: review-1-target
from

Conversation

Projects
None yet
2 participants
@thibaudcolas
Copy link
Owner

thibaudcolas commented Jan 18, 2019

This PR was created to help review the code of https://github.com/noripyt/react-streamfield before integration into Wagtail (wagtail/wagtail#4942). As of now, it covers v0.6.10 of react-streamfield.

For the high-level summary, see noripyt#4.

BertrandBordage added some commits Aug 20, 2018

@@ -0,0 +1,2 @@
/node_modules/
/.idea/

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

I would suggest ignoring dist in git, and only having it in the npm package (via the package.json's files attribute, as of now).

</head>
<body>
<h1>React StreamField demo</h1>
<h2>1 block type</h2>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

I think those examples would greatly benefit from using Storybook. It's meant for exactly this type of "UI development / playground / examples / demos" scenario, is really easy to set up (by comparison to a custom Webpack + dev server setup), and comes with very nice features built-in:

  • Hot reloading of React components and styles
  • Navigation tree to easily move between examples
  • Plugin ecosystem of development helper panels, say for testing across multiple screen sizes, or i18n testing, or accessibility checks. And many more.

I switched the Draftail examples to it a few weeks ago and it was well worth it. See http://demo.draftail.org/storybook for a demo.

}
],
"value": [{"type": "title", "value": "Wagtail is awesome!"}]
}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

Could you take a moment to document this schema somewhere? Up until now it’s been Wagtail’s internals, but now that it's an important part of this package’s API I would find it important to have a high-level outline of the different attributes, their type and purpose, whether they need to be unique or not, which are optional and which aren't.

This doesn't necessarily need to be written docs, it could be as simple as adding comments to BlockDefinitionType and linking to it from the README / copying its code there.

"blockDefinitions": [
{
"key": "title",
"icon": "<i class='fas fa-heading fa-fw'></i>",

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

It would be nice to use SVGs (symbols or inline path definitions) for the icons in the examples, instead of FontAwesome, as we are actively trying to remove icon fonts from Wagtail.

"html": "<input type='checkbox' name='field-__ID__' />"
}
],
"html": "As you can see by this text, it’s possible <strong>to insert some HTML</strong> before or after the contained blocks. <BlocksContainer /> You can even have multiple times the same blocks container. <BlocksContainer /> Can’t think of a case where that would be useful, but still, it’s possible if you really want it."

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

I'm pretty sure I'll have other occasions to understand this better elsewhere in the code, but <BlocksContainer /> doesn't look like valid HTML and even if it's harmless I'd rather encourage people to only do things that validate as HTML5.

border-color: $error-border-color;
}
}
&:hover, &:focus, &:focus-within {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

:focus-within still doesn't have that good browser support. Is this functionality important enough to justify adding a --focus class in JS, and add styles based on the class instead?

&:hover, &:focus, &:focus-within {
> .block-container {
border-color: $error-border-color-focus;
> header {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

This should use a class name instead of a bare element name.

border-radius: $border-radius;
background: white;
transition: border-color $hover-transition-duration ease-in-out;
transition-property: border-color, box-shadow;

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

I think this would be clearer as:

transition: border-color $hover-transition-duration ease-in-out, box-shadow $hover-transition-duration ease-in-out;
&.SIMPLE {
display: flex;
flex-flow: row nowrap;
> header {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

Same comments as above for bare elements and BEM state syntax, and also for all following styles.

flex: 0 0 auto;
padding: $header-padding;
h3 {
position: sticky;

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 18, 2019

Author Owner

This seems to only be a minor effect (but nice nonetheless), but I think it would be worth stating in the project's README that a position: sticky polyfill might be desirable, as IE11 doesn't support it.

This should also be added in the Wagtail docs, over at https://docs.wagtail.io/en/v2.4/contributing/developing.html?highlight=Sticky.

@connect((state, props) => {
const {fieldId, parentId, blockId} = props;
let blockDefinitions;
if (parentId === null) {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

For strings that don't have a reason to be empty, it would be more idiomatic to write the conditional as if (parentId).

event.preventDefault();
event.stopPropagation();
if (this.hasChoice) {
this.setState({open: !this.state.open});

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

Generally it's considered a bad habit to update state based on other state like this, because setState can batch updates and thus the UI can end up in an inconsistent state.

The alternative is to use an updater function, which guarantees the state to be up to date when it runs:

this.setState((state, props) => ({open: !state.open});

See https://reactjs.org/docs/react-component.html#setstate

event.stopPropagation();
this.props.addBlock(this.props.index,
event.target.closest('button').value);
this.toggle(event);

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

As far as I can see toggle already does preventDefault and stopPropagation, so those calls shouldn't be necessary above.

event.preventDefault();
event.stopPropagation();
this.props.addBlock(this.props.index,
event.target.closest('button').value);

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

IE11 doesn't implement closest. The README should mention that an element-closest polyfill is needed for it. Luckily, Wagtail already has it.

if (isNA(icon)) {
return null;
}
return <span className="icon" dangerouslySetInnerHTML={

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

This is way too generic of a class name, and is guaranteed to clash with existing or future code. Please use something more specific, or consider namespacing classes (e.g. streamfield-icon).

} = this.props;
return (
<aside>
<div className="actions">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

Too generic of a class name.

duplicateBlock: () => duplicateBlock(fieldId, blockId),
}, dispatch);
})
class BlockActions extends (React.Component) {

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

Extra parens?

{sortableBlock ?
<React.Fragment>
<button onClick={this.moveUpHandler}
title="Move up"

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

This should be localized / provided by Wagtail.

}

triggerCustomEvent(name, data=null) {
triggerCustomEvent(ReactDOM.findDOMNode(this), name, data);

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

From what I can see below, the data parameter is never used.

<Draggable draggableId={id} index={index}
type={`${fieldId}-${parentId}`}>
{(provided, snapshot) => (
<article className={className}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 19, 2019

Author Owner

This doesn't seem like the appropriate tag for this scenario.

"bugs": {
"url": "https://github.com/noripyt/react-streamfield/issues"
},
"license": "BSD",

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

"BSD" isn't a valid license identifier according to npm (SPDX identifiers only). Looks like it should be BSD-3-Clause to be the same as Wagtail.

<React.Fragment>
{button}
<AnimateHeight height={this.panelHeight} easing="ease-in-out"
contentClassName="add-panel">

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

Testing this, I see that AnimateHeight renders all of its content in order to calculate its height. Do you know if there is a way around this? If there are many AddButton instances on a page, this is a lot of DOM nodes for not much value.

{Object.entries(this.groupedBlockDefinitions).map(
([group, blockDefinitions]) => (
<div key={group}>
<h4 className="group-name">{group}</h4>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

This h4 should not be rendered if group is an empty string.

<AnimateHeight className="draggable-container"
height={this.draggableHeight}
onAnimationEnd={this.onDraggableContainerAnimationEnd}>
{this.wrapSortable(blockContent)}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

Same comment as with AddButton – it seems like a big waste on the performance side to render all of these, and have them hidden with styles. For pages with lots of StreamField, this means the browser will come to a crawl even if only some of them are open.

</button>
<button onClick={this.moveDownHandler}
title="Move down"
className={this.isLast ? 'disabled' : null}>

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

The buttons should have the disabled attribute, rather than this being achieved with CSS.

<AnimateHeight height={this.height} easing="ease-in-out"
className="content-container"
contentClassName={className}>
{content}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

Same concern as other AnimateHeight comments above.

}
}
this.runInnerScripts();
}

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

This is a potential source of memory leaks, due to the event listener (and the mutation observer)? in bindChange. There should be a corresponding componentWillUnmount that removes the event listeners, and disconnects the mutation observers.

<DragDropContext onDragEnd={this.onDragEnd}>
<BlocksContainer fieldId={id} />
<input type="hidden" name={id}
value={JSON.stringify(generatedValue)} />

This comment has been minimized.

@thibaudcolas

thibaudcolas Jan 20, 2019

Author Owner

I find this strange that StreamField takes the responsibility for storing its data like this, and patches over Wagtail's storage in its componentWillMount. I think it would be better if the component exposed an onChange callback (or similar) in its props, so Wagtail can take charge of storing the data.

@thibaudcolas thibaudcolas changed the title react-streamfield code review for upcoming Wagtail integration Code review for upcoming Wagtail integration Jan 21, 2019

@thibaudcolas thibaudcolas referenced this pull request Jan 21, 2019

Open

Code review #4

0 of 74 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment