sanusart / react-dropdown-select

Customisable dropdown select for react
https://react-dropdown-select.netlify.app/
MIT License
349 stars 83 forks source link

FEATURE REQUEST: Only use the property pointed to by valueField for component values instead of full objects #291

Closed devnull69 closed 1 year ago

devnull69 commented 1 year ago

Currently, you need to provide full objects as values for the component and you also get back full objects when selecting values from the component.

I would like to have an optional feature added to the component. So that there are two cases

  1. (default case) use the full objects
  2. (optional case) use the valueField property only. It should (of course) be unique, so it identifies a single specific object from the options, but this is the responsibility of the developer using the component

This option should then be valid for the values during the full lifecycle of the component, so onChange should provide only the valueField properties of the selected values.

devnull69 commented 1 year ago

Hello again,

I hope I'm not getting on your nerves :-) I would really like to participate in this component project, because I really like your component and would like to use it more in future projects

Therefore, I wanted to setup my local environment to be able to change and test the library in my fork. I have NO experience whatsoever with creating and/or testing a react library, but generally I consider myself an experienced developer

Can you maybe help me with this? What is your local setup in order to develop and test the library? Can you maybe point me to a resource where this is explained?

Thanks!

sanusart commented 1 year ago

Hey, LOL, not at all.

In general. the setup is just

And that is it. Every change to the /src folder will be reflected in the local docs site running.

If you can wait a bit, I am currently setting up storybook. It will be way easier to develop upon. I plan to merge a basic Storybook branch later today so maybe it is worth to wait.

devnull69 commented 1 year ago

Ah ok ... so in DOCS there is actually already a test project included? That's good news

I will try with the current version ... and maybe come back to the Storybook one later. Storybook is another "thing" I never used before :-)

devnull69 commented 1 year ago

Bad luck. I only get a bunch of errors when doing npm install in the docs folder of the freshly cloned fork

npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: react-simple-code-editor@0.11.0
npm WARN Found: react@17.0.2
npm WARN node_modules/react
npm WARN   react@"^17.0.2" from the root project
npm WARN   17 more (@emotion/core, @emotion/react, @emotion/styled, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react@"^16.0.0" from react-simple-code-editor@0.11.0
npm WARN node_modules/react-simple-code-editor
npm WARN   react-simple-code-editor@"^0.11.0" from react-live@2.4.1
npm WARN   node_modules/react-live
npm WARN
npm WARN Conflicting peer dependency: react@16.14.0
npm WARN node_modules/react
npm WARN   peer react@"^16.0.0" from react-simple-code-editor@0.11.0
npm WARN   node_modules/react-simple-code-editor
npm WARN     react-simple-code-editor@"^0.11.0" from react-live@2.4.1
npm WARN     node_modules/react-live
npm WARN ERESOLVE overriding peer dependency
npm WARN While resolving: react-simple-code-editor@0.11.0
npm WARN Found: react-dom@17.0.2
npm WARN node_modules/react-dom
npm WARN   react-dom@"^17.0.2" from the root project
npm WARN   8 more (@gatsbyjs/reach-router, gatsby, gatsby-link, ...)
npm WARN
npm WARN Could not resolve dependency:
npm WARN peer react-dom@"^16.0.0" from react-simple-code-editor@0.11.0
npm WARN node_modules/react-simple-code-editor
npm WARN   react-simple-code-editor@"^0.11.0" from react-live@2.4.1
npm WARN   node_modules/react-live
npm WARN
npm WARN Conflicting peer dependency: react-dom@16.14.0
npm WARN node_modules/react-dom
npm WARN   peer react-dom@"^16.0.0" from react-simple-code-editor@0.11.0
npm WARN   node_modules/react-simple-code-editor
npm WARN     react-simple-code-editor@"^0.11.0" from react-live@2.4.1
npm WARN     node_modules/react-live
npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR!
npm ERR! While resolving: react-virtualized@9.22.3
npm ERR! Found: react@17.0.2
npm ERR! node_modules/react
npm ERR!   react@"^17.0.2" from the root project
npm ERR!   peer react@">=16.3.0" from @emotion/core@10.3.1
npm ERR!   node_modules/@emotion/core
npm ERR!     peer @emotion/core@"^10.0.28" from @emotion/styled-base@10.3.0
npm ERR!     node_modules/@emotion/styled-base
npm ERR!       @emotion/styled-base@"^10.3.0" from @emotion/styled@10.3.0
npm ERR!       node_modules/react-dropdown-select/node_modules/@emotion/styled
npm ERR!         @emotion/styled@"^10.0.27" from react-dropdown-select@4.8.3
npm ERR!         node_modules/react-dropdown-select
npm ERR!     @emotion/core@"^10.0.27" from react-dropdown-select@4.8.3
npm ERR!     node_modules/react-dropdown-select
npm ERR!       react-dropdown-select@"^4.8.3" from the root project
npm ERR!     1 more (@emotion/styled)
npm ERR!   16 more (@emotion/react, @emotion/styled, @emotion/styled-base, ...)
npm ERR!
npm ERR! Could not resolve dependency:
npm ERR! peer react@"^15.3.0 || ^16.0.0-alpha" from react-virtualized@9.22.3
npm ERR! node_modules/react-virtualized
npm ERR!   react-virtualized@"^9.22.3" from the root project
npm ERR!
npm ERR! Conflicting peer dependency: react@16.14.0
npm ERR! node_modules/react
npm ERR!   peer react@"^15.3.0 || ^16.0.0-alpha" from react-virtualized@9.22.3
npm ERR!   node_modules/react-virtualized
npm ERR!     react-virtualized@"^9.22.3" from the root project
npm ERR!
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.
npm ERR!
npm ERR!
npm ERR! For a full report see:
npm ERR! C:\Users\Thomas\AppData\Local\npm-cache\_logs\2023-07-28T08_47_35_562Z-eresolve-report.txt

npm ERR! A complete log of this run can be found in:
npm ERR!     C:\Users\Thomas\AppData\Local\npm-cache\_logs\2023-07-28T08_47_35_562Z-debug-0.log

Do you know anything about this one?

sanusart commented 1 year ago

Try to runnpm i with extra --legacy-peer-deps flag

e.g. npm i --legacy-peer-deps

devnull69 commented 1 year ago

Ok ... I managed to get it working

And I also implemented the new option, and I called it "fullObjectValues" with TRUE as default value. Everything seems to be working fine on my side

Nevertheless, I'm not familiar with your testing setup. Even committing my changes to my fork doesn't work because of failing SNAPSHOT tests (in my case for the CONTENT component).

What can I do (what can we do) to bring both parts together?

Thanks Thomas

sanusart commented 1 year ago

Hey, testing is done with Jest. Snapshots can be regenerated with npm t -- -u if they have a valid dif. Regarding fullObjectValues - I am not sure one needed, maybe we can just detect if it is array of strings or not.

On Tue, Aug 1, 2023 at 1:43 AM Thomas Theiner @.***> wrote:

Ok ... I managed to get it working

And I also implemented the new option, and I called it "fullObjectValues" with TRUE as default value. Everything seems to be working fine on my side

Nevertheless, I'm not familiar with your testing setup. Even committing my changes to my fork doesn't work because of failing SNAPSHOT tests (in my case for the CONTENT component).

What can I do (what can we do) to bring both parts together?

Thanks Thomas

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/291#issuecomment-1659290043, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRVVLTZJSQTQVCPINCDXTAYJPANCNFSM6AAAAAA2Z6OGUI . You are receiving this because you commented.Message ID: @.***>

devnull69 commented 1 year ago

I think the option is still necessary, because the initial values array can also be empty

sanusart commented 1 year ago

So if empty then nothing to map? No?

On Tue, Aug 1, 2023 at 12:15 PM Thomas Theiner @.***> wrote:

I think the option is still necessary, because the initial values array can also be empty

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/291#issuecomment-1659909047, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRVBK5LZN75XLOOWP7TXTDCKNANCNFSM6AAAAAA2Z6OGUI . You are receiving this because you commented.Message ID: @.***>

devnull69 commented 1 year ago

I don't understand your question ....

You said that the new option "fullObjectValues" might not be necessary because it could just be detected if the values array is an array of strings or an array of objects. But what if the values array is empty initially, you would not be able to detect the type then

sanusart commented 1 year ago

But the question is - do you need to detect them before there are even some values?

On Tue, Aug 1, 2023 at 12:19 PM Thomas Theiner @.***> wrote:

I don't understand your question ....

You said that the new option "fullObjectValues" might not be necessary because it could just be detected if the values array is an array of strings or an array of objects. But what if the values array is empty initially, you would not be able to detect the type then

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/291#issuecomment-1659916076, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRS7DHKZCGXJH3SPZRTXTDC3LANCNFSM6AAAAAA2Z6OGUI . You are receiving this because you commented.Message ID: @.***>

devnull69 commented 1 year ago

Yes ... because the moment you want to select one of the options, the component must know if the value should be the full selected object or just one string property out of the selected object

sanusart commented 1 year ago

Well. If you expect values to be array of string - you would also do the options as array of strings, right - so based on the options you can know if what we dealing with here is ['','',''] or [{},{},{}]

devnull69 commented 1 year ago

Ehm ... no

The options can (and should) still be any kind of object. The relevant properties (label and value) are already determined by "valueField" and "labelField" respectively.

So on the screen, you would still see absolutely the same thing, but the values (initial values and selected values) will not be the full selected objects but rather only the valueField properties of those objects. Ideally, those valueField properties can UNIQUELY identify each object of the options.

Imagine these options with valueField="id" and labelField="name":

[{
   "id": 1,
   "name": "Thomas",
   "email": "thomas.theiner@gmx.de",
   "address": {
      "street": "Main St."
   }
}, {
   "id": 2,
   "name": "Martin",
   "email": "martin.wolters@whatever.com",
   "address": {
      "street": "Market St."
   }
}]

Now, if you selected the entry with name "Thomas" only, the value array would look like this

[1]

rather than

[{
   "id": 1,
   "name": "Thomas",
   "email": "thomas.theiner@gmx.de",
   "address": {
      "street": "Main St."
   }
}]

But because you know that "1" is the unique id of the first option, you can still retrieve it, but the value array is a lot slimmer and easier to handle in "remote transports" (REST calls etc.)

sanusart commented 1 year ago

No, I mean you are working on a scenario where one would input: options as ['one','two','three'] and if the user selects one and three - the values should be ['one','three'].

This is what I understand from the feature. Otherwise why would we want to return output that does not correspond to input.

On Tue, Aug 1, 2023 at 3:20 PM Thomas Theiner @.***> wrote:

Ehm ... no

The options can (and should) still be any kind of object. The relevant properties (label and value) are already determined by "valueField" and "labelField" respectively.

So on the screen, you would still see absolutely the same thing, but the values (initial values and selected values) will not be the full selected objects but rather only the valueField properties of those objects. Ideally, those valueField properties can UNIQUELY identify each object of the options.

Imagine these options with valueField="id" and labelField="name":

[{ "id": 1, "name": "Thomas", "email": @.", "address": { "street": "Main St." } }, { "id": 2, "name": "Martin", "email": @.", "address": { "street": "Market St." } }]

Now, if you selected the entry with name "Thomas" only, the value array would look like this

[1]

rather than

[{ "id": 1, "name": "Thomas", "email": @.***", "address": { "street": "Main St." } }]

But because you know that "1" is the unique id of the first option, you can still retrieve it, but the value array is a lot slimmer and easier to handle in "remote transports" (REST calls etc.)

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/291#issuecomment-1660202454, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRTJKWGPTDXNR6XYTDDXTDYBJANCNFSM6AAAAAA2Z6OGUI . You are receiving this because you commented.Message ID: @.***>

devnull69 commented 1 year ago

It corresponds ... but is much leaner, smaller, slimmer

The value [1] corresponds to the object with "id": 1 in my example, because the valueField "id" uniquely identifies each object in the options

The options will NOT change, only the values change.

I have finished the implementation and also the tests are now running fine. I will send a PULL request and you can take a look at it yourself

devnull69 commented 1 year ago

I sent the PULL REQUEST

sanusart commented 1 year ago

Thank you I will try to check later today.

On Tue, Aug 1, 2023, 5:01 PM Thomas Theiner @.***> wrote:

I sent the PULL REQUEST

— Reply to this email directly, view it on GitHub https://github.com/sanusart/react-dropdown-select/issues/291#issuecomment-1660397183, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACBLRQVOFZ6UG6SEUT2LADXTED5LANCNFSM6AAAAAA2Z6OGUI . You are receiving this because you commented.Message ID: @.***>