Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Fixes #1256 #1769

Merged
RaymondLim merged 4 commits into from
Oct 10, 2012
Merged

Fixes #1256 #1769

RaymondLim merged 4 commits into from
Oct 10, 2012

Conversation

@DennisKehrig
Copy link
Contributor

@DennisKehrig DennisKehrig commented Oct 4, 2012

Fixes #1256: Deleting a file should remove entry from inline editor

This one could use a thorough review...

@ghost ghost assigned RaymondLim Oct 4, 2012
RaymondLim
RaymondLim reviewed Oct 8, 2012
View reviewed changes
src/document/TextRange.js Outdated
Copy link
Contributor

@RaymondLim RaymondLim Oct 8, 2012

Choose a reason for hiding this comment

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

Is it a typo? thought?

Copy link
Contributor Author

@DennisKehrig DennisKehrig Oct 8, 2012

Choose a reason for hiding this comment

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

Yes! It's supposed to say "though". Good catch!

@RaymondLim
Copy link
Contributor

RaymondLim commented Oct 8, 2012

Done with initial review.

@DennisKehrig
Copy link
Contributor Author

DennisKehrig commented Oct 8, 2012

Thank you for the review! Much appreciated!

Personally I'm most skeptical about forwarding an event as the "cause" of another event. It feels fishy somehow. What are your thoughts on this?

@RaymondLim
Copy link
Contributor

RaymondLim commented Oct 8, 2012

I can't think of other elegant alternatives that will let you distinguish between "deleted" and "out-of-sync". So I'm ok with forwarding the "cause". It is harder to understand it at first. I was almost to question you on how we get "deleted" as the cause, but later I figured out by doing global search.

@DennisKehrig
Copy link
Contributor Author

DennisKehrig commented Oct 10, 2012

Thanks a lot for your review, much appreciated! I updated the comments as per your suggestions. Unless I overlooked anything, that's it from me for now.

@RaymondLim
Copy link
Contributor

RaymondLim commented Oct 10, 2012

Looks good. Merging

RaymondLim added a commit that referenced this pull request Oct 10, 2012
@RaymondLim
Merge pull request #1769 from DennisKehrig/dk/issue-1256
444ae4d 
Fixes #1256
@RaymondLim RaymondLim merged commit 444ae4d into adobe:master Oct 10, 2012
@core-ai-bot core-ai-bot mentioned this pull request Aug 29, 2021
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@RaymondLim RaymondLim

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Deleting a file should remove entry from inline editor

2 participants

@DennisKehrig @RaymondLim