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

[RelationshipList] A field for a relation-many that display a table #2885

Closed
wants to merge 12 commits into from

Conversation

Nikodermus
Copy link
Contributor

@Nikodermus Nikodermus commented May 4, 2020

Disclaimer

This PR is the same as #2646, I just moved my branch to do multiple contributions


Why

The idea behind this, is for some fields with large relations to many, text chips are insufficient and hard to consume. That field display can grow out of control and doesn't provide detail if needed.

How

This would display a customizable table, with special support for links and booleans. For a more in depth explanation of the customization see packages/fields/src/types/RelationshipList/README.md

What it looks like

Kapture 2020-04-02 at 18 36 43

Before merge

  • Pray you guys will think this is a good idea
  • Revert auth changes in demo/blog
  • Create "display" option in Relationship, (table)
  • Move changes to Relationship, under "tableOptions"
  • Run changeset

@changeset-bot
Copy link

changeset-bot bot commented May 4, 2020

💥 No Changeset

Latest commit: e650d58

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

If these changes should be published to npm, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@timleslie
Copy link
Contributor

Hi @Nikodermus,

Thanks for this PR, I think it's a really interesting idea, and definitely something we've wanted to implement for a while, but haven't landed on a solution that we felt would work nicely.

I think there's a few different things we need to work out for this PR.

I think the general approach is good, there's a decent amount of re-use from the Relationship field, but I think we could step that up even more.

Your Implementation class only adds one API change, adding the innerFields config option. You could simplifiy the Implementation class by having it subclass the Relationship Implementation class and updating just the constructor and extendAdminMeta methods.

The views for Field and Filter seem to duplicate a lot of patterns (e.g. RelationshipSelect that are used in other places in the admin UI). Are there ways to factor out this duplication?

The other question is about where this feature should best live. In general we are trying to keep the core fields implemented within @keystonejs/fields trimmed down, and more complex field types live in their own package (e.g. @keystonejs/field-content, @keystonejs/field-markdown, etc). I think we would probably want to put this into it's own package as well.

We're also looking at ways of including custom views into field definitions more easily than the current custom field type pattern. This would allow people to definite custom views, like the relationship list and mix them into the core field types, which might be simpler than creating whole new field types.

The other question that this PR raises is how much of the admin-ui component library is "public" and should be re-used to build custom views, and how much of it should be considered an implementation details. We're trying to limit what we expose from the admin UI package, because having more exported components makes it harder to maintain and leads to more major version bumps when we need to change things. It would be interesting to see how much of this new relationship view could be implemented without needing to export more stuff from the admin UI package.

My final thought for now on this is that the one thing we know about the Relationship field type is that it is infinitely more complex than all of the other scalar field types (e.g. Text, Integer, etc), and there are always going to be special cases for how we handle relationships vs how we handle other field types. So with that in mind, it could be the case that this PR serves as an example of how we might be able to provide a few different types of relationship view that people can choose from depending on what's most appropriate for their use case.

So all that said I would encourage you to keep working on this PR, and focussing on removing any duplication. I'm not yet sure how/where this implementation would best fit into the system, but it's definitely something we're interested in having somewhere 👍

@yavuzirfanoglu
Copy link

Hello there.
I loved the idea @Nikodermus I am a newbie in Keystone. I understand that current Relationship Field, keeps items in a set, so we can not sort them.

I know your PR is not about it, but is it technically possible to sort Relationship Items by unique index?

Thanks.

@gautamsi
Copy link
Member

@Nikodermus do you mind if I fix this and add to contrib project?

@Nikodermus
Copy link
Contributor Author

@gautamsi please do it, I don't have the bandwidth to work in this

@stale
Copy link

stale bot commented Dec 9, 2020

It looks like there hasn't been any activity here in over 6 months. Sorry about that! We've flagged this issue for special attention. It wil be manually reviewed by maintainers, not automatically closed. If you have any additional information please leave us a comment. It really helps! Thank you for you contribution. :)

@stale stale bot added the needs-review label Dec 9, 2020
@stale stale bot removed the needs-review label Apr 8, 2021
@bladey
Copy link
Contributor

bladey commented Apr 8, 2021

Thanks for this PR @Nikodermus.

Keystone 5 has officially moved into active maintenance mode as we push towards the next major new version Keystone Next, you can find out more information about this transition here.

In an effort to sustain the project going forward, we're cleaning up and closing old PRs such as this one.

However, you are welcome to create a PR under the Keystone 5 repository.

Please let me know if you have any questions.

@bladey bladey closed this Apr 8, 2021
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.

5 participants