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

feat(netlify-cms-widget-relation): use react-select and add support for multiple entries #1936

Merged
merged 7 commits into from
Feb 19, 2019
Merged

feat(netlify-cms-widget-relation): use react-select and add support for multiple entries #1936

merged 7 commits into from
Feb 19, 2019

Conversation

alexandernanberg
Copy link
Contributor

Summary

Closes #1061

Related to #1928 but this only focuses on the existing relation widget. The behavior should be the same as the old one. Before I continue on this I'd like some confirmation if this can "live" beside the other PR, did this pretty quickly to play around a bit.

Test plan

So far just manual testing – should probably add some unit tests

@verythorough
Copy link
Contributor

verythorough commented Dec 5, 2018

Preview proposed changes to netlifycms.org in the link below:

Built with commit 23fdc64

https://deploy-preview-1936--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 5, 2018

Preview proposed changes to the CMS demo site in the link below:

Built with commit 23fdc64

https://deploy-preview-1936--cms-demo.netlify.com

@leonardodino
Copy link
Contributor

Horray! 🎉

was going to do something like this today, you just saved me a bunch of effort (:

@erquhart
Copy link
Contributor

Going to hold onto this until we finish implementing tests for the widgets, working on it as we speak.

@leonardodino
Copy link
Contributor

I left some off-band review here: https://github.com/alexandernanberg/netlify-cms/pull/1

it's mostly compatibility with the code I implemented earlier in #1928

thumbs-up for testing it, especially with something like react-testing-library that allows the changing of implementation

@erquhart
Copy link
Contributor

That's exactly what we're using 👍

@tomrutgers tomrutgers mentioned this pull request Dec 12, 2018
28 tasks
@shaunbent
Copy link

Just ran into a use case where I needed to use multiple select in the relationship component. So happy to see this PR open. Awesome work @alexandernanberg.

Looking forward to seeing this merged.

@codedre
Copy link

codedre commented Jan 28, 2019

Is there an estimate of when this might be merged in?

@erquhart
Copy link
Contributor

erquhart commented Feb 3, 2019

@alexandernanberg apologies for the delay - this is a 1000% improvement, merging. Please keep 'em coming!

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Ah, did find one bug - an existing value doesn't show when you load an entry, only after focusing the relation field: https://deploy-preview-1936--cms-demo.netlify.com/#/collections/kitchenSink/entries/a-big-entry-with-all-the-things

@alexandernanberg
Copy link
Contributor Author

No worries! Huh weird - I'll have a look asap (probably in a day or two)

@barthc
Copy link
Contributor

barthc commented Feb 4, 2019

@alexandernanberg any reason why you omitted the relation widget's fieldsmetadata in this PR?

@erquhart
Copy link
Contributor

erquhart commented Feb 4, 2019

@barthc I don't see any mention of fieldsmetadata?

@alexandernanberg
Copy link
Contributor Author

I guess the relation widget should be updated to work like the select widget (with the changes in #2054)?

Though I'm not sure that I'm 100% onboard with the changes in that PR in retrospect, kind of makes sense to keep the same type always to me. Uncertain if it will break some Gatsby configs because you expect to receive an empty array instead of null 🤔

@alexandernanberg
Copy link
Contributor Author

Also the bug seems to have gone away... not sure why ¯_(ツ)_/¯

@erquhart
Copy link
Contributor

erquhart commented Feb 8, 2019

Good point on null vs. string in the other PR. Strictness of GraphQL is still something we (as a community) have to figure out.

It looks like the preview isn't working here, but it is working on master. There was always an issue where the preview sometimes didn't show initially, but you can test by typing "json" into the field and selecting the "This is a JSON front matter post" entry.

You'll notice on master it loads the preview, but in this PR updating the value still doesn't show the preview.

@erquhart
Copy link
Contributor

erquhart commented Feb 8, 2019

Ah, it's the thing @barthc was talking about, figured I was missing something when he said that 🙄

The original onChange call passes in an object for fieldsMetadata: https://github.com/netlify/netlify-cms/pull/1936/files#diff-1c6e2a499213935b7c1b04dbb4e1ca1cL110

The new onChange will need to do the same, as this provides the connection between the selection value and the related entry.

Note: in case you don't have it, the Redux dev tools extension is super helpful for troubleshooting.

@alexandernanberg
Copy link
Contributor Author

I think the metadata should work like before now. But I found some other bugs that I've been unable to solve... if you search for eg. "YAML" and select that post, then try to select another one without searching the selectedValue will be null. That's because queryHits.get just returns the hit for "YAML".

Not sure how we'd handle this 🤔

@erquhart
Copy link
Contributor

Gotcha - the previews are definitely fixed, thanks for that 👍👍

@barthc would you be up for reviewing this and weighing in on the queryHits.get question?

@barthc
Copy link
Contributor

barthc commented Feb 13, 2019

A couple of things. The kitchen sinkrelation widget preview doesn't work on initial load here. On master everything inside componentDidMount and componentDidUpdate does the trick, looks like you ommited those too.

That's because queryHits.get just returns the hit for "YAML".
Not sure how we'd handle this

I suggest you store the initial defaultOptions and concat them here. The idea is to always make the defaultOptions avalaible for selection inside the getSelectedValue function.

@barthc
Copy link
Contributor

barthc commented Feb 15, 2019

@alexandernanberg if you don't mind, I would like to push a commit, that would address my comments.

@alexandernanberg
Copy link
Contributor Author

alexandernanberg commented Feb 15, 2019

@barthc That would be great since I don't have time right now. You should have push access to my fork now!

It would be nice it we could solve the intial preview without updating the state imo, but that might not be possible? 🤔

@barthc
Copy link
Contributor

barthc commented Feb 18, 2019

@alexandernanberg @erquhart review?

Copy link
Contributor Author

@alexandernanberg alexandernanberg left a comment

Choose a reason for hiding this comment

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

I'm thinking since we have all the "options/hits" already loaded in allOptions, does it still make sense to query them?

LGTM otherwise 👍

onChange(fromJS(value), {
[field.get('name')]: {
[field.get('collection')]: {
[last(value)]: !isEmpty(selectedOption) && last(selectedOption).data,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how the underlying mechanics works here but wouldn't it be better to have this as a list somehow – instead of just picking the last item

Copy link
Contributor

@barthc barthc Feb 19, 2019

Choose a reason for hiding this comment

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

The last item will always be the newly added item on the list, so we are guaranteed that all selected item will have their metadata in store. We cannot use list here because that would mean using List() as key here [last(value)].

@barthc
Copy link
Contributor

barthc commented Feb 19, 2019

I'm thinking since we have all the "options/hits" already loaded in allOptions, does it still make sense to query them?

Yes we have all hits, but we still need to select the items that matches the query using the fuzzy module. The best we can do is cache results for backend listAllEntries which should improve performance and help fix #1357 but that will be handled in a separate PR.

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

Thanks for all of your work on this @alexandernanberg! Thanks for reviewing and wrapping it up @barthc, and extra thanks for the tests, they help a lot!!

@sanscheese
Copy link

Does this mean this should now be available for everyone use? Is there an example of code for the config.yml?

@barthc
Copy link
Contributor

barthc commented Feb 25, 2019

@sanscheese you would need to add the multiple options to the widget multiple: true just like you would do for select widget.

@erquhart we need to merge this #2112 to update the doc.

@sanscheese
Copy link

sanscheese commented Feb 25, 2019

@barthc Hmm. I tried that. It looks like the field is still using react-autosuggestion rather than react-select for me. (So I can type to find a tag)

This is the field I've setup in my config.yml

- label: "Tags"
    name: "tags"
    widget: "relation"
    collection: "tags"
    displayFields: ["title"]
    searchFields: ["title"]
    valueField: "title"
    multiple: true

My admin/index.html file is using https://unpkg.com/netlify-cms@^2.0.0/dist/netlify-cms.js

I can see the new multi-relation using the kitchenSink in the dev-test admin of this netlify-cms repo but have noticed it's config.yml is using widget: 'relationKitchenSinkPost which made me think is still not released for full use yet?

@barthc
Copy link
Contributor

barthc commented Feb 25, 2019

@erquhart is the new relation widget released yet?

@erquhart
Copy link
Contributor

erquhart commented Feb 25, 2019

No, releasing today. Was working on some way to provide next releases instead of going straight to latest, but it's quite complicated with Lerna, independent versions, and conventional commits.

@sanscheese
Copy link

This is amazing timing for me! Congrats, and great work on this! 😀

@erquhart
Copy link
Contributor

Update: this was released in 2.5.0 🎉

@FredrikSigvartsen
Copy link

Update: this was released in 2.5.0 🎉

I must say: It looks and works very well! Great work!

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.

9 participants