-
Notifications
You must be signed in to change notification settings - Fork 56
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
Parse html into known blocks #100
Parse html into known blocks #100
Conversation
…/parse-html-into-blocks
Mainly to fix `flow` errors. Flow also doesn't like curried annotation :[
src/store/reducers/index.js
Outdated
@@ -108,6 +108,10 @@ export const reducer = ( | |||
blocks.splice( index, 1 ); | |||
return { blocks: blocks, refresh: ! state.refresh }; | |||
} | |||
case ActionTypes.BLOCK.PARSE: { | |||
const parsed = action.parse(action.payload) | |||
return { blocks: parsed, refresh: state.refresh }; |
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.
Why not import parse
in this file instead of passing it down every time you want to use it?
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 idea was to avoid a hard coded dependency.
But for simplicity I'd agree that it would be better to import it directly. 👍
src/store/reducers/index.js
Outdated
@@ -108,6 +108,10 @@ export const reducer = ( | |||
blocks.splice( index, 1 ); | |||
return { blocks: blocks, refresh: ! state.refresh }; | |||
} | |||
case ActionTypes.BLOCK.PARSE: { | |||
const parsed = action.parse(action.payload) | |||
return { blocks: parsed, refresh: state.refresh }; |
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 whole idea of using Redux store is to take advantage of immutability. when you update reference to the blocks
, you can treat it as an indicator if something has changed. This makes usage of refresh
boolean flag obsolete as it tracks the state which can be derived from blocks
.
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.
src/store/actions/index.js
Outdated
|
||
export const parseBlocksAction: BlockActionType = (payload, parser) => ( { | ||
type: ActionTypes.BLOCK.PARSE, | ||
uid: '', |
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.
uid
can be removed, it is never used. When parsing blocks from HTML you can get multiple blocks and as far as I see, it replaces all content.
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 I remove uid
, flow
gets angry saying that 'uid' is missing in object literal
, since BlockActionType
expects a uid
property.
That's one reason why I thought that a new reducer operating in the same store might be a good idea.
Instead, I'm inclined to define a ParseActionType
to keep it simple, remove uid
and keep flow
happy.
style={ styles.htmlView }> | ||
{ this.serializeToHtml() } | ||
style={ styles.htmlView } | ||
onChangeText={ ( html ) => this.props.html = html }> |
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.
You should never mutate props
, you should track changes to html
in state
. In this case, you rather should pass down callback to this component which will update html
outside.
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.
My original idea was to keep track of the html
updates in a simple property this.html
, but I kept getting a flow
warning about it not being part of the BlockManager
type.
I tried to add it as part of the component state
but on every update, the InputView is reloaded and it feels weird from a UX point of view (but it works).
What would be the advantage of lifting the html
state up vs keeping it in the component's state?
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 also tried what is described here (lifting state up), but the jumpiness remains.
@@ -44,6 +51,10 @@ export default class BlockManager extends React.Component<PropsType, StateType> | |||
}; | |||
} | |||
|
|||
componentDidMount() { | |||
this.serializeToHtml(); |
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 function returns string, so it should be assigned to a variable.
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 was an experiment and I forgot to remove it 😞
Thanks for catching it!
This is because you mutate
I had a similar concern. I wouldn't spend too much time doing it yourself using your own store. All those issues are already solved in web version of Gutenberg. Speaking myself, I would focus of integrating functionalities and we can improve the general architecture by integrating with the existing |
…/parse-html-into-blocks
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 work @etoledom :) I only left a couple of minor comments 👍
src/store/reducers/test.js
Outdated
@@ -4,6 +4,9 @@ | |||
|
|||
import { reducer } from './'; | |||
import * as actions from '../actions/'; | |||
import { registerCoreBlocks } from '@gutenberg/core-blocks'; | |||
|
|||
registerCoreBlocks() |
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.
Small nit - should we register the blocks here or within the specific test, when there's only one test using them? deferring to @hypest here as a related comment is taking place in #88 (comment)
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 was following the example from block-parser-code.test.js
and the others, but if it makes more sense to put them in the test case itself, no problem!
What it feels weird to me is that we are testing parsing two times, one from the parse
function directly and other from the redux action indirectly (just the more
block for now). Maybe we can find a way to do it just once.
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.
hi! - I just moved a similar case to beforeAll()
as per https://github.com/wordpress-mobile/gutenberg-mobile/pull/88/files/e55637838c3f16fefc5ef8de028855ebeaac209f#r209576735 in commit a04c5e9 - wdyt?
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.
Sounds good! I'll go for it. Thank you
src/store/actions/index.js
Outdated
|
||
export const parseBlocksAction: ParseActionType = payload => ( { | ||
type: ActionTypes.BLOCK.PARSE, | ||
payload: payload, |
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.
Small nit - can we rename payload
to html
as an indication of what the payload is supposed to be about?
Tests are failing but they're passing for me locally when running |
FWIW, the tests fail for me locally with the same errors seen on Travis. cc @mzorz |
I got the same tests error locally when I updated the branch from master, it was solved when I run |
@mzorz I pushed a couple more commits. I hope those handle your comments :) About the tests, I think I merged wrongly the reference to the |
…/parse-html-into-blocks
Very nice @etoledom ! 🙇 been testing it quite a bit today
Given #95 has been closed without merging, due to a better solution to our specific case having been found in #108, I think we still need to address this here. Knowing this might be a bit too much to ask for to do in this PR alone, I went ahead and tried something on my own in #116 (I just didn't feel suggesting something without making sure it would work first was good enough, and so I ended up with a PR 😄). Hope that's alright! Please feel free to comment / indicate otherwise. Ccing @hypest |
HTML parser - restart datasource after full parse of html
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.
With #116 merged, all is good here! :)
Use Bintray mirror of RN when building in Jitpack
Part of #99
This PR adds the ability of parsing the html string in the html view back into blocks.
For simplicity, the current implementation will just parse the known blocks, and anything that is outside of those blocks will be ignored.
An example on iOS:
On Android, this currently won't work, but this issue was taken care of by #95. I tested a fast merge and it works:
There are many details I wasn't sure about, I chose the simplest solutions for all of them, but I hope this PR is a good place to discuss them. The most important ones are:
The current reducer seems to me that is meant for actions over a single block, but the
parse
action will regenerate the whole list of blocks.I was tempted to create another reducer specifically for parser, but it immediately made this implementation (and PR) grow on complexity, so I opted to add an extra action to the current reducer.
An interesting question I had was where to keep the state of the text that the user is editing.
My first test was to store it as part of the component state (as the example shows), but for each time the state changes, a new instance of the view is created. This makes the whole UX very bad when editing on a scrolled position.
I also though about the redux store, but I wasn't sure about it and it was more complex than the next option.
I opted to simply store the edited text in a variable.