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

#372 allow sortable(false) on fields in list view #984

Closed

Conversation

RichardBradley
Copy link
Contributor

@RichardBradley RichardBradley commented Mar 17, 2016

Fixes #372

(UPDATE: please ignore the rest of this comment, it refers to an old version of this PR. Jump straight to the comment starting "OK, I have updated this pull request to include the necessary changes")

As discussed on that thread, this PR is much easier if I don't have to also modify admin-config.
However, it does look rather out of place next to all the fields with function getters & setters, so maybe a version which spans the two respositories would be better.

This version works for me though.

(Another benefit of the getter/setter approach is that it allows a "builder" style. This version is awkward to use:

articles.listView()
   .fields(
       nga.field('id'),
       function() {
            var headline = nga.field('headline');
            // Headline is an "analyzed" string field, so cannot be sorted in ES
            // https://www.elastic.co/guide/en/elasticsearch/guide/current/multi-fields.html
            headline.disableSort = true;
            return headline;
        }(),
        nga.field('publication_time', 'datetime'),
        nga.field('type')
    ])

whereas the getter/setter version would look like

articles.listView()
   .fields(
       nga.field('id'),
       nga.field('headline')
            // Headline is an "analyzed" string field, so cannot be sorted in ES
            // https://www.elastic.co/guide/en/elasticsearch/guide/current/multi-fields.html
            .disableSort(true),
        nga.field('publication_time', 'datetime'),
        nga.field('type')
    ])

)

@RichardBradley
Copy link
Contributor Author

Taking a step back, why is admin-config in a separate git repos to ng-admin?
It would simplify the project a lot to merge them together; they are already very tightly coupled, so having them in separate repositories just adds administrative overhead.

@fzaninotto
Copy link
Member

admin-config is also used by react-admin, that's why it's independent.

Please update admin-config with a related PR to use a more usual function syntax.

@Fr4ncx
Copy link

Fr4ncx commented Mar 22, 2016

.disableSort(true), Not work in my panel, why?

@RichardBradley
Copy link
Contributor Author

.disableSort(true), Not work in my panel, why?

Because this enhancement has not yet been implemented.
The issue is still marked "open" at the top -- subscribe to #372 for updates and try again when it is marked "closed".

@RichardBradley
Copy link
Contributor Author

Please update admin-config with a related PR to use a more usual function syntax.

@fzaninotto Is there a guide on how best to do this?

I'll make the changes to admin-config; do I then need to bump its version and update the dependency reference in ng-admin/package.json? How will that work if the PR to admin-config has not been merged / published yet?

I suppose I should publish it locally somehow (how?) and then have my local ng-admin depend on my locally modified admin-config. Then I'll be pushing a version of ng-admin which will work only for me (until the PR in admin-config is merged), which seems messy.

@RichardBradley RichardBradley changed the title #372 allow disableSort on fields in list view #372 allow sortable(false) on fields in list view Apr 4, 2016
@RichardBradley
Copy link
Contributor Author

OK, I have updated this pull request to include the necessary changes to admin-config. Please could you take another look and merge if you are happy?

This now depends on marmelab/admin-config#65 -- please merge and release that first, or this will fail.

I have bumped the admin-config version and increased the version required by ng-admin accordingly.

I have not included changes to build/, as requested in the README

@RichardBradley
Copy link
Contributor Author

(The CI build has failed because it cannot find the not-yet-published version of admin-config on which this change depends. This is inevitable with the current git repos structure, as briefly discussed above.)

@RichardBradley
Copy link
Contributor Author

I have changed the syntax from the proposed "disabledSort(true)" to ".sortable(false)", as it better matches the other Field properties like "editable"

@@ -5,21 +5,21 @@ A field is the representation of a property of an entity.
* [General Field Settings](#general-field-settings)
Copy link
Member

Choose a reason for hiding this comment

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

please revert these changes, they are not related to this PR, and break the ng-admin book on GitBook (i.e. the official doc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, will do.

The links are currently broken on GitHub, see e.g. https://github.com/marmelab/ng-admin/blob/master/doc/reference/Field.md#-date-field-type

They do work on the "GitBook" version, you're right: http://ng-admin-book.marmelab.com/doc/reference/Field.html#-date-field-type

It's a shame that the two differ. I hadn't spotted the "GitBook" version; I'd been referring to the documentation on GItHub directly up until now. If the GitBook version is the official one, it should win, of course.

@RichardBradley
Copy link
Contributor Author

I have pushed a new version with the requested changes (and rebased to account for other changes in the meantime).

@fzaninotto fzaninotto added this to the 1.0 milestone Nov 18, 2016
@fzaninotto
Copy link
Member

This PR is OK, just needs rebasing

@fzaninotto
Copy link
Member

Superseded by #1260

@fzaninotto fzaninotto closed this Dec 1, 2016
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