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

Writing Flow: Avoid using state for tracking arrow key navigation #2894

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Oct 5, 2017

Related: https://github.com/WordPress/gutenberg/pull/2424/files#r142135955

This pull request seeks to refactor the WritingFlow component to eliminate state which could be set on arrow keypresses. Assigning state will incur a rerender, and for the intended usage, we do not need state to be reflected in the render result. Instead, for tracking the "should move" behavior, an instance property is sufficient and avoids any render.

Open questions:

I will need to look back through the old pull request, but at a glance I'm struggling to understand why we need to bind to both key down and key up, particularly because binding to key up causes a laggy feeling to the arrow movement.

Testing instructions:

Verify that there are no regressions in using arrow keys (left, right, up, down) to navigate from one input to the next / previous, e.g. between two paragraph blocks.

@aduth aduth added the Blocks label Oct 5, 2017

@aduth aduth requested a review from youknowriad Oct 5, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 5, 2017

Codecov Report

Merging #2894 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2894      +/-   ##
==========================================
+ Coverage   33.92%   33.93%   +0.01%     
==========================================
  Files         190      190              
  Lines        5695     5693       -2     
  Branches      999      999              
==========================================
  Hits         1932     1932              
+ Misses       3183     3181       -2     
  Partials      580      580
Impacted Files Coverage Δ
editor/writing-flow/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a89801b...757c845. Read the comment docs.

codecov bot commented Oct 5, 2017

Codecov Report

Merging #2894 into master will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2894      +/-   ##
==========================================
+ Coverage   33.92%   33.93%   +0.01%     
==========================================
  Files         190      190              
  Lines        5695     5693       -2     
  Branches      999      999              
==========================================
  Hits         1932     1932              
+ Misses       3183     3181       -2     
  Partials      580      580
Impacted Files Coverage Δ
editor/writing-flow/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a89801b...757c845. Read the comment docs.

@@ -17,13 +17,12 @@ const { UP, DOWN, LEFT, RIGHT } = keycodes;
class WritingFlow extends Component {
constructor() {
super( ...arguments );
this.zones = [];

This comment has been minimized.

@youknowriad

youknowriad Oct 6, 2017

Contributor

😅

@youknowriad

youknowriad Oct 6, 2017

Contributor

😅

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 6, 2017

Contributor

I'm struggling to understand why we need to bind to both key down and key up

some context here #2424 (comment)

Basically, I first tried to only use keyDown which was great until you release the key (keyUp) which was causing the caret to move unexpectedly and I couldn't figure out what was moving the caret on keyup.

So I decided to use keyUp as a workaround but we still need the keyDown because we need to know the caret position before any move happens.

Contributor

youknowriad commented Oct 6, 2017

I'm struggling to understand why we need to bind to both key down and key up

some context here #2424 (comment)

Basically, I first tried to only use keyDown which was great until you release the key (keyUp) which was causing the caret to move unexpectedly and I couldn't figure out what was moving the caret on keyup.

So I decided to use keyUp as a workaround but we still need the keyDown because we need to know the caret position before any move happens.

@youknowriad

🚢 it

@gziolo gziolo merged commit 5dbbbce into master Oct 6, 2017

3 checks passed

codecov/project 33.93% (+0.01%) compared to a89801b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/arrow-navigation-instance-property branch Oct 6, 2017

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