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.

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

Fix modified indicator #1204

Merged
merged 3 commits into from
Nov 17, 2016
Merged

Conversation

lgeiger
Copy link
Member

@lgeiger lgeiger commented Nov 17, 2016

This addresses part of #1165

But it still fails in a interesting way:
modified

@@ -36,7 +36,7 @@ export function createTitleFeed(state$) {
const modified$ = state$
.map(state => state.document.get('notebook'))
.scan((prev, notebook) => ({
modified: prev.notebook === notebook,
modified: prev.notebook !== notebook,
Copy link
Member Author

Choose a reason for hiding this comment

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

Immutable.is(prev.notebook, notebook) === false

will behave the same.

@rgbkrk
Copy link
Member

rgbkrk commented Nov 17, 2016

I just realized... We're missing logic for saved. All we know is if it was modified from last time.

The logic should be

  modified: saved.notebook !== current.notebook

Yet we don't have saved.notebook available here.

@codecov-io
Copy link

codecov-io commented Nov 17, 2016

Current coverage is 91.57% (diff: 100%)

Merging #1204 into master will increase coverage by <.01%

@@             master      #1204   diff @@
==========================================
  Files            57         57          
  Lines          1530       1531     +1   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1401       1402     +1   
  Misses          129        129          
  Partials          0          0          

Powered by Codecov. Last update 26d7b69...313cf11

@jdetle
Copy link
Member

jdetle commented Nov 17, 2016

Is this still WIP?

@rgbkrk
Copy link
Member

rgbkrk commented Nov 17, 2016

It fixes a bug yet doesn't fix the modified feature. It can be merged, we've just got some work ahead of us. I think I know what to do for a follow up PR.

@ivanov
Copy link
Member

ivanov commented Nov 17, 2016

Merging, thank you!

@ivanov ivanov merged commit 4437165 into nteract:master Nov 17, 2016
@lgeiger lgeiger deleted the fix-modified-indicator branch November 17, 2016 21:54
@lock
Copy link

lock bot commented Apr 3, 2018

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.

@lock lock bot locked and limited conversation to collaborators Apr 3, 2018
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.

None yet

5 participants