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 facet values in ascending order as default #2581

Merged
merged 5 commits into from
Jun 27, 2017
Merged

Conversation

yhoonkim
Copy link
Contributor

Fix #2553

(As a future work, I think we should allow user to sort facet as they want.)

@yhoonkim yhoonkim requested a review from kanitw June 27, 2017 00:38
Copy link
Member

@kanitw kanitw left a comment

Choose a reason for hiding this comment

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

A few comments -- you can add sort: 'ascending' | 'descending'.

I think you haven't npm run build:examples.

cross: true
}}: {})
}
},
...(isCrossedFacet ? {sort: {
...(hasRow && hasColumn ? {sort: {
Copy link
Member

Choose a reason for hiding this comment

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

You always produce sort and order so writing it this way is better:

{
  sort: {
    field: [].concat(
      hasRow: ? [this.field(ROW, {expr: 'datum'})] : [],
      hasColumn: ? [this.field(COLUMN, {expr: 'datum'})] : []
    )
    order: [].concat(
      hasRow: ? [ (this.facet.row.header && this.facet.row.header.sort) || 'ascending'] : [],
      hasColumn: ? [ ... ] : []
    )
}

from: {data: model.getName(channel)},
sort: {
field: field(layoutHeader.facetFieldDef, {expr: 'datum'}),
order: 'ascending'
Copy link
Member

Choose a reason for hiding this comment

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

(facetFieldDef.header && facetFieldDef.header.sort) || 'ascending'

@kanitw kanitw mentioned this pull request Jun 27, 2017
10 tasks
@kanitw
Copy link
Member

kanitw commented Jun 27, 2017

I just fix a minor thing in the declaration. Let's merge when test finishes running.

@kanitw kanitw merged commit aaf54f9 into master Jun 27, 2017
@kanitw kanitw deleted the yh/fix-facet-header branch June 27, 2017 22:45
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.

2 participants