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

Admin UI: add adminIsReadOnly property to enable fields to appear read-only #2841

Closed
wants to merge 2 commits into from

Conversation

lucatk
Copy link

@lucatk lucatk commented Apr 27, 2020

Adds a property adminIsReadOnly to fields config, which enables the user to make certain fields appear read-only (although they remain changeable through GraphQL) inside the admin UI.
This leverages the same view as the cells in list tables.

Especially useful for data fields, which are only to be changed by the underlying system, and not by actual users of the Admin UI thus diminishing the chance of accidental corruption of data in those fields.

@changeset-bot
Copy link

changeset-bot bot commented Apr 27, 2020

🦋 Changeset is good to go

Latest commit: 5581936

We got this.

This PR includes changesets to release 2 packages
Name Type
@keystonejs/app-admin-ui Minor
@keystonejs/fields Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@gautamsi
Copy link
Member

This is already pending for long
#2258

@lucatk
Copy link
Author

lucatk commented Apr 27, 2020

@gautamsi Thanks for the heads-up, you're right. Basically the same change.
I'd love to see one of them get merged though, what's holding it back?

@gautamsi
Copy link
Member

I guess it is not aligned with current roadmap that is why

@MadeByMike
Copy link
Contributor

MadeByMike commented Apr 30, 2020

This has come up enough times that I think we should enable people do it. I like the idea of using the cell view over passing this prop around every field.

This idea is good but we need a way of separating AdminUI configuration from the other field configurations. This is really why discussion on this has stalled in the past. A few of us feel that the best API for this is actually not config on the field itself but the right APIs to customise fields in general.

I want it to be easier to simply override the rendering of this field. i.e. replace the field or the cell or any view. At the moment this is not too hard, create a custom field that extends the existing one and replace the field view. Perhaps we could add a read only example to the demos here: https://github.com/keystonejs/keystone/tree/master/demo-projects/custom-fields/

In the long run perhaps a better option might be to allow customisation from the field config like this:

keystone.createList('Example', {
  fields: {
    example: { 
      type: Text,
      views: {
        Field: require.resolve('./path-to/read-only/component')
      },
    },
  },
});

Other options available under views would include, Controller, Filter, and Cell.

I'd much rather figure out this API and provide examples of how to create read-only and other types of adminUI customisations such as #2646.

The right API here will meet all the different requirements for field customisation without adding more AdminUI concerns to the field properties.

Thoughts?

@gautamsi
Copy link
Member

gautamsi commented May 1, 2020

I thought about using Cell view but there are some caveat:

  • There is no Cell view for many fields, as they are not intended to be rendered as cell perhaps.
  • if you try to render them as text then it will be raw markdown and html content.
  • adding isReadOnly to field view also enables several other scenario.
    • one of them is if you have no update access, you can see them as read only and still see the rendered content.
    • This can also enable feature like initial read only field (prevent accidentally editing) but you can have edit button (overlay to field) to explicitly edit them if needed

@timleslie
Copy link
Contributor

A lot of the blockers for this is that our current mechanism for passing configuration from the list/field definitions into the admin UI client code is somewhat opaque. This makes it hard for us to evaluate the impact of API changes like this. I'm making a bunch of changes that you can probably see going through at the moment which are intended to shed light on the way we pass through this config, which will in turn allow us to better assess these changes. As Mike said, these are really useful features if we can get the APIs right, so hopefully we will be able to get some movement on these PRs soon 👍

@timleslie
Copy link
Contributor

This can be closed once #2258 goes through (In final review now).

@timleslie
Copy link
Contributor

#2258 is now in 👍

@timleslie timleslie closed this May 21, 2020
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.

None yet

4 participants