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

Sort table view columns alphabetically #2825

Merged
merged 4 commits into from
Mar 18, 2015
Merged

Conversation

viddo
Copy link
Contributor

@viddo viddo commented Mar 18, 2015

Fixes #2638

  • Changed the sortSchema method to sort "default" items alphabetically i.e. items that should not be on a specific position according to the predefined priorities
  • Removed the optional priorities param since it was not used anywhere

screen shot 2015-03-18 at 15 48 19

Nicklas Gummesson added 2 commits March 18, 2015 11:03
Secondary to predefined priorities
Test didn’t actually passed when run in browser, re-implemented sort
comparison to work as desired and expected
@viddo
Copy link
Contributor Author

viddo commented Mar 18, 2015

@javisantana ok?

@@ -178,8 +178,8 @@
return name && name.replace(/"/g, '');
},

sortSchema: function(priorities) {
this.set('schema', cdb.admin.CartoDBTableMetadata.sortSchema(this.get('schema'), priorities));
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove the param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Removed the optional priorities param since it was not used anywhere

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, I'm probably agree, but not sure why I did that, probably we were using that param at some point from somewhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured, in general I think it's better to remove unused code when doing changes though than leaving it around "just in case" :)

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, agree

@javisantana
Copy link
Contributor

🇪🇸

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

viddo added a commit that referenced this pull request Mar 18, 2015
Sort table view columns alphabetically
@viddo viddo merged commit 8b6fc60 into master Mar 18, 2015
@viddo viddo deleted the 2638-fix-table-schema-sort branch March 18, 2015 15:18
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.

Columns are no longer alphabetically ordered in the table view
3 participants