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

sortableFields have default sorting option #4774

Closed
wants to merge 3 commits into from

Conversation

KancerEzeroglu
Copy link
Contributor

closes #3715

Summary
We couldn't set the default sortable field from the config file.
With my changes, it can happen.
You can specify the sortable fields like below:

sortable_fields:
   - field: title
      direction: 'Descending'
   - field: date

Test plan
I haven't added the tests in the code (I've removed those that belong to the sortable fields test).
I would like to be sure my direction is right.

A picture of a cute animal (not mandatory but encouraged)

giphy

@KancerEzeroglu KancerEzeroglu requested a review from a team December 28, 2020 15:29
@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Dec 29, 2020
@KancerEzeroglu KancerEzeroglu marked this pull request as draft December 30, 2020 09:36
@KancerEzeroglu KancerEzeroglu marked this pull request as ready for review December 30, 2020 13:58
Copy link
Contributor

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Hi @KancerEzeroglu and thank you for this.
A few comments:

  1. We would like to keep the config backwards compatible. With this PR anyone with existing sortable_fields will have to update their config. We can add migration code here:
    https://github.com/netlify/netlify-cms/blob/567438fdd1aa775fd09a6a5de13e79c9d783e0d6/packages/netlify-cms-core/src/actions/config.js#L283

  2. The current suggested API allows setting multiple fields as the default which is not supported by the UI. We can use something like:

sortable_fields: { names: ['title','date'], default: name, direction: descending }
  1. I think it would be better to initialize the default sort in the reducer to avoid the UI refreshing after initial load.

WDYT?

@KancerEzeroglu
Copy link
Contributor Author

Sorry for the late reply @erezrokah
I will work on what you have written and get back to you.
Thank you for your review.

@KancerEzeroglu
Copy link
Contributor Author

KancerEzeroglu commented Jan 18, 2021

The current suggested API allows setting multiple fields as the default which is not supported by the UI. We can use something like:
sortable_fields: { names: ['title','date'], default: name, direction: descending }

In the future, we can enable users to do multiple sorts.
So I think the configuration should be based on this.
What do you think about that @erezrokah?

@erezrokah
Copy link
Contributor

In the future, we can enable users to do multiple sorts.
So I think the configuration should be based on this.
What do you think about that @erezrokah?

Hi @KancerEzeroglu and sorry again for the late reply.

How about enforcing using our schema validation that only a single field has a direction?

@erezrokah
Copy link
Contributor

Closing this as stale. Let me know if you'd like me to re-open it

@erezrokah erezrokah closed this Mar 10, 2021
@jlev
Copy link

jlev commented Mar 16, 2021

It would be really nice to have default sort ordering for date fields. With a lot of posts, having the default be date ascending is not a great user experience...

Is the direction schema validation a blocker to getting this merged? If so, I can take a look at it if you point me in the right direction.

@erezrokah
Copy link
Contributor

Is the direction schema validation a blocker to getting this merged? If so, I can take a look at it if you point me in the right direction.

Hi @jlev the blockers for the PR are:

  1. It breaks existing configurations.
  2. It allows you to configure something that doesn't work (can be solved by schema validation)

Please note that the CMS remembers the user's preference for the sort if that helps and having a default sort field can impact performance quite a lot as it requires the CMS to fetch all posts on initial load.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sortableFields should have default sorting options
3 participants