-
Notifications
You must be signed in to change notification settings - Fork 58
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 post title to Gutenberg Mobile #450
Changes from 28 commits
8c89460
6c5d701
eb361cb
d80dba6
b788654
16181fd
739cf56
ea8288c
818325f
fc425ca
99fe984
e4e4ed5
dcca2c6
271da87
469c3fc
46a20ca
efceada
b53123a
ebaf39e
6f723db
e31e844
7ca5656
f487db8
bec3537
e98f6fc
1e62bdd
10e112a
c29bdca
87dfddf
5c97308
21f39b6
1d64d25
1062340
1e6776a
6968c62
3293e97
0b5e9b1
e7b3b8f
38ed202
ec1418f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,13 +28,19 @@ type PropsType = { | |
onSelect: string => mixed, | ||
clearSelectedBlock: void => void, | ||
onAttributesUpdate: ( string, mixed ) => mixed, | ||
title: string, | ||
initialHtml: string, | ||
setupEditor: ( mixed, ?mixed ) => mixed, | ||
clientId: string, | ||
}; | ||
|
||
class AppContainer extends React.Component<PropsType> { | ||
type StateType = { | ||
title: string, | ||
}; | ||
|
||
class AppContainer extends React.Component<PropsType, StateType> { | ||
lastHtml: ?string; | ||
lastTitle: ?string; | ||
|
||
constructor( props: PropsType ) { | ||
super( props ); | ||
|
@@ -47,8 +53,13 @@ class AppContainer extends React.Component<PropsType> { | |
type: 'draft', | ||
}; | ||
|
||
this.props.setupEditor( post ); | ||
this.state = { | ||
title: props.title, | ||
}; | ||
|
||
props.setupEditor( post ); | ||
this.lastHtml = serialize( parse( props.initialHtml ) ); | ||
this.lastTitle = props.title; | ||
|
||
if ( props.initialHtmlModeEnabled && ! props.showHtml ) { | ||
// enable html mode if the initial mode the parent wants it but we're not already in it | ||
|
@@ -91,15 +102,24 @@ class AppContainer extends React.Component<PropsType> { | |
if ( this.props.showHtml ) { | ||
this.parseBlocksAction( this.props.editedPostContent ); | ||
} | ||
|
||
const html = serialize( this.props.blocks ); | ||
RNReactNativeGutenbergBridge.provideToNative_Html( html, this.lastHtml !== html ); | ||
const hasChanges = this.state.title !== this.lastTitle || this.lastHtml !== html; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noticed that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a regression or is this a problem with the original logic (before my changes) too? The reason I'm asking is that the condition and logic haven't changed much in terms of what happens when you edit the content, and if this issue was part of the original logic, fixing it is outside of the purpose of this PR. |
||
|
||
RNReactNativeGutenbergBridge.provideToNative_Html( html, this.state.title, hasChanges ); | ||
|
||
this.lastTitle = this.state.title; | ||
this.lastHtml = html; | ||
}; | ||
|
||
toggleHtmlModeAction = () => { | ||
this.props.onToggleBlockMode( this.props.rootClientId ); | ||
}; | ||
|
||
setTitleAction = ( title: string ) => { | ||
this.setState( { title: title } ); | ||
}; | ||
|
||
updateHtmlAction = ( html: string ) => { | ||
this.parseBlocksAction( html ); | ||
}; | ||
|
@@ -122,9 +142,11 @@ class AppContainer extends React.Component<PropsType> { | |
createBlockAction={ this.createBlockAction } | ||
serializeToNativeAction={ this.serializeToNativeAction } | ||
toggleHtmlModeAction={ this.toggleHtmlModeAction } | ||
setTitleAction={ this.setTitleAction } | ||
updateHtmlAction={ this.updateHtmlAction } | ||
mergeBlocksAction={ this.mergeBlocksAction } | ||
isBlockSelected={ this.props.isBlockSelected } | ||
title={ this.state.title } | ||
/> | ||
); | ||
} | ||
|
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.
I think instead of holding title in current component's state we might need to update the central store with the new title and inject it into props within
withSelect
just like we do for isBlockSelected, selectedBlockIndex, blocks etc.In order to achieve that we can imitate what web is doing in components/post-title/index.js, looks like they are calling editPost function from 'core/editor' with a parameter like { title: "new Title" } whenever a title is updated. They are making use of withDispatch to inject onUpdate method to props.
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.
This brings up two questions in my head.
The first one is: As I understand, this is to keep in line with the rest of the architecture, or is there a technical reason why this is necessary before merging the PR?
The second question is related to the first one: Is this urgent or can I work on it incrementally? I'd like if possible to work on more atomic PRs to avoid monolithic ones which can become very complex very fast.
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.
I think the number of lines would probably decrease after using the store because we'll be getting rid of propagating title change through props and we'll remove the state, and considering that it'll require same tests it doesn't look like an incremental change to me. The technical reason could be that currently the store doesn't know about the title at all, so I feel like we'll need to address this sooner or later but I don't want to block or get in the way of anything. Maybe other reviewers have other opinions on this.
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.
Working incrementally is 100% about simplifying the steps.
My reasoning is:
Right now, Aztec is being able to query the title as it is.
Again, I'm definitely not advocating to do things the wrong way... just proposing that we split the steps. It's only a difference in methodology.
From what I can tell, we should also get the feature sooner this way.
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.
We can open a separate issue and move fwd on this I think. And It'd be good to have opinion of @hypest also. I'll keep on reviewing this in parallel I couldn't finish it yet.
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.
👋. I think it's OK to work by splitting to separate steps, even if a step will actually revise code written from a previous step. Reason is that it can help scope down the change (note, not the same as minimizing lines-of-code change) and review it easier/faster.
What I would recommend though is to at least add some inline comments in the places that we're pretty sure we'll touch in a follow up PR. For example, a suitable comment in this case is that we'd like to move the state to the Redux Store (
data
) so, this local state is only a intermediate step. Hope this makes sense!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.
I still don't have the knowledge to know what'll need to be changed or where. Sorry about that!
But since we'll need to wait a bit on the Android part I'll try branching off this PR and pushing forward the changes in parallel.
Sounds good?
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.
No prob! Let's just handle the cases already commented about, cool?
@Tug , can you have a look at the Gutenberg-web codebase to figure out where is the post title stored? Is it in the Redux store or elsewhere? @diegoreymendez let's add a comment about this when Tug gets insight on this one, cool?
That's fine, sure. I'm mainly trying to care about the points already brought up during this review so we don't lose sight of them. We can always handle/fix/update things in follow up PRs.
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.
the title is part of the post:
We don't need to use the component state for that, it's in the global store.
We can then access/modify it using
getEditedPostAttribute( 'title' )
andeditPost( { title } )
and it's added to the list of edits!See the exemple here: https://github.com/WordPress/gutenberg/tree/master/packages/editor/src/components/post-title/index.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.
Thanks @Tug. While I didn't tackle this in this PR, I'm now opening a new one to take care of it.
This was very valuable and insightful.