Skip to content

Fix navigation reload#297

Merged
Gaurav0 merged 8 commits intoember-cli:masterfrom
piotrpalek:fix-navigation-reload
Dec 21, 2015
Merged

Fix navigation reload#297
Gaurav0 merged 8 commits intoember-cli:masterfrom
piotrpalek:fix-navigation-reload

Conversation

@piotrpalek
Copy link
Copy Markdown
Contributor

This fixes issue #290, but it depends on these changes to codemirror.

@piotrpalek
Copy link
Copy Markdown
Contributor Author

I've (temporarily) changed the CodeMirror dependency to the modified version so tests will pass on Travis.

@piotrpalek
Copy link
Copy Markdown
Contributor Author

I changed contentsChanged on the file-editor-column component, it didn't make sense that contentsChanged wouldn't fire when the content in the column actually changed. Now the gist controller gets to decide what to do depending on a flag the new action fires.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think you need to change the test description to match the new behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm on mobile now so I could miss something, but what do you mean? The description is correct(imo) on the main test method above, and this assertion would say that it expected a true value (if it would fail). Did you mean to add that it expects a true value in this assertion?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of "contentChanged action was called", "contentChanged was called with isUserChange=true"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah sure, I thought the title would be enough but I'll add it

@Gaurav0
Copy link
Copy Markdown
Contributor

Gaurav0 commented Dec 20, 2015

This looks really good! Just need to fix up minor nits.

Comment thread package.json Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's ok if we can't, but I'd much prefer to make sure the upstream changes land before merging.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes, please at least open a PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure will do, it was just so you can see that the tests actually pass :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've made a PR to CodeMirror

@piotrpalek
Copy link
Copy Markdown
Contributor Author

Changes landed upstream so I updated package.json to reflect that, if everything is all right should I rebase to a single commit?

@Gaurav0
Copy link
Copy Markdown
Contributor

Gaurav0 commented Dec 21, 2015

No need, and actually I'm not a fan of rewriting history.

@Gaurav0
Copy link
Copy Markdown
Contributor

Gaurav0 commented Dec 21, 2015

👍

Gaurav0 added a commit that referenced this pull request Dec 21, 2015
@Gaurav0 Gaurav0 merged commit 2bacf0b into ember-cli:master Dec 21, 2015
@piotrpalek piotrpalek deleted the fix-navigation-reload branch December 21, 2015 20:43
@piotrpalek
Copy link
Copy Markdown
Contributor Author

👍

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants