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

Create typing reducer and keep flag on inserting blocks #1812

Merged
merged 3 commits into from Jul 14, 2017

Conversation

Projects
None yet
5 participants
@iseulde
Member

iseulde commented Jul 8, 2017

See #1561.

To test: type in a text block and press enter twice to create a new text block. No UI should appear.

@iseulde iseulde requested a review from youknowriad Jul 8, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 8, 2017

Coverage Status

Coverage decreased (-0.2%) to 17.662% when pulling 01b1da7 on try/editor-level-typing into b450568 on master.

Coverage Status

Coverage decreased (-0.2%) to 17.662% when pulling 01b1da7 on try/editor-level-typing into b450568 on master.

@youknowriad

This works pretty well. Two things I think we should do (maybe later)

  • Handle Tab and Arrow keys: The behaviour is not inconsistent between the types of blocks, if we navigate to a text block, the borders stay hidden but if we navigate to an image block or a "blocky" block they appear again (and stay even if we navigate to a text block again)
  • Maybe hide the placeholder of the text block if we're typing
@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jul 10, 2017

Member

@youknowriad It seems like scroll is triggering it sometimes? Not sure why, only mouse move should trigger it. Maybe we should add a buffer at some point to only trigger mouse move after # px.

Member

iseulde commented Jul 10, 2017

@youknowriad It seems like scroll is triggering it sometimes? Not sure why, only mouse move should trigger it. Maybe we should add a buffer at some point to only trigger mouse move after # px.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 10, 2017

Coverage Status

Coverage decreased (-0.2%) to 17.662% when pulling d052d2d on try/editor-level-typing into d157b5e on master.

Coverage Status

Coverage decreased (-0.2%) to 17.662% when pulling d052d2d on try/editor-level-typing into d157b5e on master.

Show outdated Hide outdated editor/selectors.js
}
return state.selectedBlock.typing;
export function isTypingInEditor( state ) {

This comment has been minimized.

@aduth

aduth Jul 10, 2017

Member

Could we name this simply isTyping? "In editor" seems implied.

@aduth

aduth Jul 10, 2017

Member

Could we name this simply isTyping? "In editor" seems implied.

This comment has been minimized.

@iseulde

iseulde Jul 10, 2017

Member

Prop name suggestions for components using it?

@iseulde

iseulde Jul 10, 2017

Member

Prop name suggestions for components using it?

This comment has been minimized.

@aduth

aduth Jul 10, 2017

Member

isTyping is fine if we don't insist to destructure props.

@aduth

aduth Jul 10, 2017

Member

isTyping is fine if we don't insist to destructure props.

This comment has been minimized.

@iseulde

iseulde Jul 10, 2017

Member

Yeah they are being destructed. So either we rename it there, or change it to non destructing.

@iseulde

iseulde Jul 10, 2017

Member

Yeah they are being destructed. So either we rename it there, or change it to non destructing.

Show outdated Hide outdated editor/selectors.js
}
return state.selectedBlock.typing;
export function isTypingInEditor( state ) {

This comment has been minimized.

@aduth

aduth Jul 10, 2017

Member

Also, DocBlock should be updated.

@aduth

aduth Jul 10, 2017

Member

Also, DocBlock should be updated.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Jul 10, 2017

Coverage Status

Coverage decreased (-0.1%) to 17.667% when pulling 0455fe1 on try/editor-level-typing into d157b5e on master.

Coverage Status

Coverage decreased (-0.1%) to 17.667% when pulling 0455fe1 on try/editor-level-typing into d157b5e on master.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jul 13, 2017

Member

Anything left here?

Member

iseulde commented Jul 13, 2017

Anything left here?

@iseulde iseulde requested a review from aduth Jul 13, 2017

@aduth

aduth approved these changes Jul 14, 2017

Code-wise looks great; nice simplification.

Seems to be a good direction for writing flow too. I agree with @youknowriad about placeholder: seems odd to appear with this flow now.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 14, 2017

Contributor

My only concern is that the setup with single line breaks feels more weird since it's not evident when you are in a new block / new paragraph. But let's try it.

What if we rename the placeholder to "New paragraph"?

Contributor

mtias commented Jul 14, 2017

My only concern is that the setup with single line breaks feels more weird since it's not evident when you are in a new block / new paragraph. But let's try it.

What if we rename the placeholder to "New paragraph"?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Jul 14, 2017

Member

@mtias I would keep the placeholders in general for consistency on backspace and focus issues with buttons if we take the focus approach. I don't mind "Write..." for now or maybe renaming as your proposing. Either placeholder text is fine for me.

Member

iseulde commented Jul 14, 2017

@mtias I would keep the placeholders in general for consistency on backspace and focus issues with buttons if we take the focus approach. I don't mind "Write..." for now or maybe renaming as your proposing. Either placeholder text is fine for me.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jul 14, 2017

Contributor

This is slightly clearer to me:

image

But ok with merging either way and testing how it feels. (It will expose the line-break / paragraph break differences.)

Contributor

mtias commented Jul 14, 2017

This is slightly clearer to me:

image

But ok with merging either way and testing how it feels. (It will expose the line-break / paragraph break differences.)

@iseulde iseulde merged commit 199295e into master Jul 14, 2017

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@iseulde iseulde deleted the try/editor-level-typing branch Jul 14, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment