Skip to content
This repository was archived by the owner on Jul 9, 2025. It is now read-only.

Conversation

@alanlong9278
Copy link
Contributor

@alanlong9278 alanlong9278 commented Oct 16, 2019

Description

Cursormove refactor.

  • Experience
    If pressing ↑ and ↓, cursor should move to previous / following node.
    If cursor is focusing on a node with siblings, pressing ← and → should navigate to siblings.
    If cursor is focusing on 'BotSays' node of a prompt type, pressing ↓ should nav to 'UserAnswers', then pressing → should nav to 'Exceptions'; if focusing on 'Exceptions', pressing ↑ should nav to 'BotSays'

Code Construction

image

TODO

Unit test

Task Item

closes #1082

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code refactor (non-breaking change which improve code quality, clean up, add tests, etc)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Doc update (document update)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have functionally tested my change

Screenshots

cursorRefactor

@alanlong9278 alanlong9278 marked this pull request as ready for review October 23, 2019 07:08
@alanlong9278 alanlong9278 force-pushed the julong/cursormoveRefactor branch 2 times, most recently from 0614e13 to 76993bf Compare October 24, 2019 08:34
@a-b-r-o-w-n
Copy link
Contributor

@yeze322 I am a little out of context with this code, so go ahead and review and merge if it is all good.

@yeze322
Copy link
Contributor

yeze322 commented Oct 29, 2019

@yeze322 I am a little out of context with this code, so go ahead and review and merge if it is all good.

In this PR, we are targeting to fix the cursor move behavior based on old mechanism (node position, not the data structure solution we discussed offline) to make sure the user experience good enough.

Alan and I was working on higher priority tasks yesterday, we will go through this PR together again this week. Now mark it as WIP until we finished.

The new cursor move algorithm is still on top of our todo list but not P0

@yeze322 yeze322 changed the title [Visual Editor]cursormove refactor [WIP][Visual Editor]cursormove refactor Oct 29, 2019
@alanlong9278 alanlong9278 force-pushed the julong/cursormoveRefactor branch from 76993bf to faa8392 Compare November 5, 2019 01:33
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2019

This pull request introduces 3 alerts and fixes 1 when merging faa8392 into d65ae3b - view on LGTM.com

new alerts:

  • 3 for Useless assignment to local variable

fixed alerts:

  • 1 for Useless conditional

@alanlong9278 alanlong9278 force-pushed the julong/cursormoveRefactor branch from faa8392 to 654cc22 Compare November 7, 2019 12:17
@alanlong9278 alanlong9278 changed the title [WIP][Visual Editor]cursormove refactor [Visual Editor]cursormove refactor Nov 11, 2019
@alanlong9278 alanlong9278 force-pushed the julong/cursormoveRefactor branch from 8858b3a to c906892 Compare November 14, 2019 09:41
@yeze322
Copy link
Contributor

yeze322 commented Nov 15, 2019

After several turns of optimizing the behavior and code structure of 'cursor move', now it has been improved a lot and in a better shape!

Cursor move is a complicated feature which requires many tricky treatments. Current code defined a good up-layer interface and detailed implementation is encapsulated in the calculate folder.

Of course we need further refactoring on calculate folder, but in this PR, alternatively, we can add UT for caculateBySchema and calculateByVector to prove they have correct behaviors.

So we have two remaining things
1 write a simple spec for explaining how it works (with example)
2 unit tests for calculate folder

@cwhitten cwhitten changed the title [Visual Editor]cursormove refactor chore: cursormove refactor Nov 16, 2019
@cwhitten
Copy link
Member

@alanlong9278 this is great work!

@cwhitten cwhitten merged commit 2d17632 into master Nov 16, 2019
@cwhitten cwhitten deleted the julong/cursormoveRefactor branch November 16, 2019 17:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants