-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Singleton List refactor #3013
Singleton List refactor #3013
Conversation
🦋 Changeset is good to goLatest commit: 9d688e2 We got this. This PR includes changesets to release 2 packages
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 |
e098330
to
fe9f3a4
Compare
fe9f3a4
to
13f7740
Compare
3089eed
to
643b02a
Compare
keystone.createList('ListWithPlugin', { | ||
fields: {...}, | ||
plugins: [ | ||
singleton(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This plugin does not need argument. We have this pattern to call the plugin when adding to list, if there is no argument this nesting of function call can be avoided. should this be a pattern or dependent on plugin whether to add as [plugin]
or as [plugin()]
.
@timleslie removed UI changes as requested |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I had a really long play with this and I have to admit that although I am glad that this PR is cut-down and much more readable it's current implementation is a little bit lacking (an that is ok).
This is a good first step that highlights some limitations. The main things are:
- GQL should be different. It makes no sense to get
allSingleton
then pluck the first item or to provide anwhere: { id: ??? }
when there is only one - Remove uneeded GQL names
- Should accept any key as a "list" name regardless of plural
- We need to do some work on how this is presented in the AdminUI
- Initial "item" should be created automatically?
- Remove from dashboard
I have a demo project I want to share shortly that I'd like to iterate on with people to inform the next steps here.
closes #2838
ref #1639
built on top of#3012#3011, see actual diff for this PR - https://github.com/keystonejs/keystone/pull/3013/files/d16188bfe8eeaec03b0fd23b6c1e9f741034494c..85ba50307242c50eaa84840e6ea2f1d8aded6e05?diff=unified&w=1This is refactor from #2838. additional argument to list plugins is moved to #3011 and disabling ui based on access control in Item Detail page footer is also moved to #3012
this PR adds TWO way of handling singleton features:. if you set{ adminConfig: { singleton: true }, access: { delete: false } }
(due to change in UI in this PR) this can give you plugin free UI version of singleton list. List view will redirect to first item in the list and you have no way to create a new item or delete the existing item.1.1 if you also setcreate
access to false and seed the list (create item onkeystone.connect
) then you do not need any plugin. you are already covered.2. above setting (except 1.1) is set automatically bysingleton
plugin. It also adds ability to create first item then you can not create more item, attempting this would give error in GraphQL. seeding is optional.UI portion of changes are split and will be in future PR.
how to use:
@timleslie: should we set
create = false
and enforce seeding to make use of this plugin (if you do not see, it is unusable). Or we leave it as recommended approach for single item list.