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

Fixes missing status on column breadcrumbs #204

Merged
merged 7 commits into from
Feb 19, 2016

Conversation

discorick
Copy link
Member

Addresses #187

Column crumb statuses were computing based on their names, however this breaks down on WIP columns when a linked repo does not share the same WIP. Fixes by moving a columns sortedIssues into the column itself, enabling us to check if the issue itself exists in the column or not.

return this.get("issue.data.current_state.name") === this.get("column.data.name");
}.property("issue.data.current_state.name", "column.data.name", "issue.data.state"),
return this.get("issue.columnIndex") === this.get("column.data.index");
}.property("issue.columnIndex", "column.data.index"),
Copy link
Member

Choose a reason for hiding this comment

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

Is this based on 'real' index or 'user input' index

'real' index == position in the array of columns
user input index == X - Column name from label

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be 'user input' index

Copy link
Member Author

Choose a reason for hiding this comment

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

But I can see where you are going with that, I am going to reimplement using real array index.

return this.get("column.sortedIssues").find((issue)=> {
return issue.get("id") === current_issue.get("id");
});
}.property("column.sortedIssues.@each"),
Copy link
Member

Choose a reason for hiding this comment

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

How does this affect the render time on the milestones view?

Copy link
Member Author

Choose a reason for hiding this comment

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

I double checked the rendering speed with the Ember inspector, they are virtually identical.

I am interested when I make the sister change with the milestone column model if we see any change. But that is a seperate PR.

@dahlbyk
Copy link
Member

dahlbyk commented Feb 18, 2016

This change looks good to me. :shipit:

@rauhryan
Copy link
Member

:shipit:

dahlbyk added a commit that referenced this pull request Feb 19, 2016
…dcrumbs

Fixes missing status on column breadcrumbs
@dahlbyk dahlbyk merged commit d6eede7 into master Feb 19, 2016
@dahlbyk dahlbyk deleted the discorick/missing_status_on_breadcrumbs branch February 19, 2016 21:48
@rauhryan
Copy link
Member

Any reason this hasn't been deployed yet?

@discorick
Copy link
Member Author

Nope, just deployed!

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