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

Show read only field by access in admin ui #2258

Merged

Conversation

gautamsi
Copy link
Member

@gautamsi gautamsi commented Jan 17, 2020

This renders readonly fields in admin-ui Item Detail/Edit page, does not affect CreateItem modal as any readonly fields do not need to be shown on create as they have no content anyways (mostly).

earlier those details are hidden and you have no ways to see the field where you have read access but no write access.

this renders them as disabled

also added isReadOnly option in field's admin config, this one is assumed in some places but the actual implementation was missing

this is how basic test project looks like when all the fields are set to isReadOnly: true

user item:
image

post item:
image

some fields are set to read only as that is the option available in external component like codemirror and slate block editor

originally if user did not have access (instead of Read Only), all of this would be removed.

this is useful in where you don't want anyone to update the content of some field (access or read only) but still render the content. simple example would be when we use atTracking/byTracking plugin, those contents are only visible when you select them in ListTable.

example:

keystone.createList('Todo', {
  fields: {
    name: { type: Text, isRequired: true },
    someReadOnlyField: {
      type: Text,
      adminConfig: {
        isReadOnly: true,
      },
      defaultValue: 'Some default value',
    },
  },
});

Tests added for read only fields being disabled.

@changeset-bot
Copy link

changeset-bot bot commented Jan 17, 2020

🦋 Changeset is good to go

Latest commit: 9a9af9a

We got this.

This PR includes changesets to release 6 packages
Name Type
@keystonejs/app-admin-ui Minor
@keystonejs/field-content Minor
@keystonejs/fields-markdown Minor
@keystonejs/fields-wysiwyg-tinymce Minor
@keystonejs/fields Minor
@keystonejs/cypress-project-access-control Patch

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 gautamsi marked this pull request as ready for review January 17, 2020 18:10
@gautamsi gautamsi force-pushed the show-read-only-field-by-access-in-admin-ui branch from c492aed to 07ae6a3 Compare January 30, 2020 19:00
@gautamsi gautamsi force-pushed the show-read-only-field-by-access-in-admin-ui branch 3 times, most recently from 691e965 to bb23890 Compare February 24, 2020 08:43
@gautamsi gautamsi force-pushed the show-read-only-field-by-access-in-admin-ui branch 2 times, most recently from e7d361d to c466706 Compare March 6, 2020 20:15
@gautamsi gautamsi mentioned this pull request Mar 31, 2020
@LiamAttClarke
Copy link
Contributor

This branch needs more ❤️.

@gautamsi
Copy link
Member Author

gautamsi commented Apr 2, 2020

unless there is any comment from maintainers I can not make any changes. already tried fixing things earlier. I am using this branch in my projects already

@gautamsi gautamsi force-pushed the show-read-only-field-by-access-in-admin-ui branch from 0ff09cc to 3d27f62 Compare April 11, 2020 22:38
@gautamsi
Copy link
Member Author

@LiamAttClarke I have fixed the conflict once more and tried to resolve failing test one last time.

@gautamsi gautamsi force-pushed the show-read-only-field-by-access-in-admin-ui branch from 3d27f62 to a661040 Compare April 11, 2020 22:50
@lucatk
Copy link

lucatk commented Apr 27, 2020

@gautamsi (sorry for the OT)
As you said you're using your own branch with this change in your projects already, could you give me a quick info on how you achieved that? When I try to use any local version of app-admin-ui (adding them locally), I always receive Module parse failed errors.
(see my issue #2836)

@gautamsi
Copy link
Member Author

they are small business companies and did not need a high performance production build so I deployed the whole project. they needed it for internal management of some processes.

I had another PR long ago which gives you a projects folder in keystone repo which is ignored in git.

I create a repo for my project and deploy the whole keystone repo including my projects folder there.

packages/fields/README.md Outdated Show resolved Hide resolved
@MadeByMike MadeByMike requested a review from jesstelford May 21, 2020 00:31
Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

This is all looking good, just a few small changes to get it over the line:

  • Slight tweaks to the language in the docs/changeset
  • One bug to fix in the Checkbox field
  • One functional change in the create modal, to completely exclude read only fields, since they have no values to display and so just clutter up the interface at that point.

packages/fields/README.md Outdated Show resolved Hide resolved
packages/app-admin-ui/client/components/CreateItemModal.js Outdated Show resolved Hide resolved
.changeset/blue-ways-work.md Outdated Show resolved Hide resolved
.changeset/blue-ways-work.md Outdated Show resolved Hide resolved
.changeset/blue-ways-work.md Outdated Show resolved Hide resolved
MadeByMike and others added 6 commits May 21, 2020 10:40
I've done this in a way that I think ensure that the hooks for readonly fields will still be triggered... did I get that right?
@MadeByMike
Copy link
Contributor

@timleslie I think all feedback is now addressed

Copy link
Contributor

@timleslie timleslie left a comment

Choose a reason for hiding this comment

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

🚢

@MadeByMike MadeByMike merged commit c2ebb51 into keystonejs:master May 21, 2020
@github-actions github-actions bot mentioned this pull request May 21, 2020
@gautamsi
Copy link
Member Author

finally, thanks all.
(that was my half century too)

@timleslie
Copy link
Contributor

Huzzah, congrats! 🎉

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.

6 participants