Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor/input helper #23

Merged
merged 3 commits into from
Jan 6, 2018
Merged

Refactor/input helper #23

merged 3 commits into from
Jan 6, 2018

Conversation

tomchentw
Copy link
Collaborator

@tomchentw tomchentw commented Dec 13, 2017 •
edited
Loading

Depends on

#24

Description

  • Migrate blink timer for BBS char and cursor to toggle class on document.body
  • Migrate inputHelper to bootstrap modal

Screenshots

screen shot 2017-12-13 at 10 24 22 pm

screen shot 2017-12-13 at 10 24 16 pm

screen shot 2017-12-13 at 10 24 11 pm

@tomchentw tomchentw self-assigned this Dec 13, 2017
@tomchentw tomchentw requested a review from robertabcd December 13, 2017 14:34
@tomchentw tomchentw force-pushed the refactor/input-helper branch from 883e05c to 5a51e7e Compare December 19, 2017 02:19
@tomchentw tomchentw changed the base branch from refactor/dev_html to dev December 23, 2017 01:28
@tomchentw
Copy link
Collaborator Author

Wow a simply change base would do the trick, awesome! @robertabcd ready for your review.

@tomchentw tomchentw force-pushed the refactor/input-helper branch from 5a51e7e to 078cc13 Compare December 23, 2017 01:54
@tomchentw tomchentw mentioned this pull request Dec 23, 2017
Merged
robertabcd
robertabcd reviewed Jan 4, 2018
View reviewed changes
Copy link
Owner

@robertabcd robertabcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't look closely, nor am I familiar with this part of code. I might over look things here.

src/components/InputHelperModal.js Outdated
ref.addEventListener("mousedown", onMouseDown);
ref.addEventListener("mousemove", onMouseMove);
ref.addEventListener("mouseup", onMouseUp);
cached.modalDialog = ref;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be deleted? Same as below.

Copy link
Collaborator Author

@tomchentw tomchentw Jan 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is redundent

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed now.

@tomchentw tomchentw force-pushed the refactor/input-helper branch from 4c213ac to 518680f Compare January 4, 2018 07:05
@tomchentw
Copy link
Collaborator Author

Fixed the code review comment, thanks!

Would someone else (maybe the author iamchucky?) be able to help review this part of code?

@tomchentw
Copy link
Collaborator Author

Is there anything I can do to help get this reviewed? Basically, what I did is:

  • Play around current input helper module, try its behavior
  • Create the same UI with React, in the same time removing the UI generation part of code from the input helper module
  • Then, the rest piece of code in the input helper module would be event listeners. Just hook them up to react & the existing system.

I have two following PRs that refactors out the modals to React, and a working branch to reactor the context menu as a whole to React in pending. I believe that should extract the major case of the application logic (pttchrome module) to React, and helps the further refactoring for:

  • Move current UI creation/event listeners to the top-level React component
  • Then, the rest piece of code would be only telnet/buf, which is the most complicated part

@robertabcd
Copy link
Owner

Sorry for the misunderstanding. I was going to merge after my comment get addressed. I didn't mean I need other reviewers for review.

@robertabcd robertabcd merged commit 3c16a12 into dev Jan 6, 2018
@tomchentw tomchentw deleted the refactor/input-helper branch January 7, 2018 02:04
@hdd60311
Copy link

hdd60311 commented Jan 7, 2018

回報一個bug就是在firefox上開啟輸入小幫手後點右上角x紐無法關閉，chrome上沒這問題

@tomchentw
Copy link
Collaborator Author

tomchentw commented Jan 8, 2018 •
edited
Loading

@hdd60311 謝謝，我來看一下

Fixed by #28

tomchentw added a commit that referenced this pull request Jan 8, 2018
@tomchentw
fix(components/InputHelperModal): onHide triggered on BUTTON
e531f31 
* #23 (comment)
tomchentw added a commit that referenced this pull request Jan 8, 2018
@tomchentw
fix(components/InputHelperModal): onHide triggered on BUTTON
9d8671e 
* #23 (comment)
@tomchentw tomchentw mentioned this pull request Jan 8, 2018
Merged
robertabcd pushed a commit that referenced this pull request Jan 8, 2018
@tomchentw @robertabcd
fix(components/InputHelperModal): onHide triggered on BUTTON
163b93e 
* #23 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertabcd robertabcd robertabcd left review comments

Assignees

@tomchentw tomchentw

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tomchentw @robertabcd @hdd60311