DiscipleTools / disciple-tools-web-components

https://jade-chebakia-17493f.netlify.app/?path=/story/kitchen-sink--kitchen-sink
GNU General Public License v2.0
3 stars 8 forks source link

Adding allowNew option to multiselects #60

Closed incraigulous closed 1 year ago

incraigulous commented 1 year ago

Adding an option to allow new option values to be added to the multi-select.

 <dt-multi-select
        allowNew
        class="create-group__input"
        label="<?php echo esc_html( $leaders_label ); ?>"
        name="leaders"
        value='<?php echo esc_attr( json_encode( $leader_ids ) ) ?>'
        options='<?php echo esc_attr( json_encode( $leader_options ) ) ?>'
   ></dt-multi-select>

Screenshot 2023-05-04 at 3 27 55 AM

Screenshot 2023-05-04 at 3 27 59 AM

netlify[bot] commented 1 year ago

Deploy Preview for jade-chebakia-17493f ready!

Name Link
Latest commit 038e62b07ac439def801a9a37ce8ae8f25d8f2d2
Latest deploy log https://app.netlify.com/sites/jade-chebakia-17493f/deploys/64536eecc0f39e0008dfcc27
Deploy Preview https://deploy-preview-60--jade-chebakia-17493f.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

cairocoder01 commented 1 year ago

@incraigulous Do multi select fields in DT allow selection of values that are not in the defined list? I know there is the Tags field type that does allow that. And the Tags component that inherits this component does implement the allowAdd feature. If the multi select field type in DT does allow new values, could you instead copy the implementation of this from the dt-tags component to this dt-multi-select component? I'm worried that the slight difference in your implementation could cause conflicts for tags and subsequently connections that inherits from the tags component.

incraigulous commented 1 year ago

Multiselect in DT fields do not allow for allowAdd and instead uses a button next to the field that triggers a modal. We would leave allowAdd to false if using the stock field setup.

I'll take a look at the tag implementation and see what I can borrow to standardize the API between the two.

cairocoder01 commented 1 year ago

@incraigulous Can you show where you see an option in the current UI to add options to a multi select field? From my quick look on a local site, only tags fields allow adding options with the Add button next to it. In this screenshot, Sources and Custom Multi Select are multi_select field types. Tags is a tags field type and is the only one that allows adding new options.

image
incraigulous commented 1 year ago

@cairocoder01 this is the field I was looking at, which may be a tag field.

Screenshot 2023-05-05 at 9 53 41 AM

cairocoder01 commented 1 year ago

That is a connection field, which inherits from the tag field. Allow add should already be available for that field.

On Fri, May 5, 2023, 5:55 PM Craig Wann @.***> wrote:

@cairocoder01 https://github.com/cairocoder01 this is the field I was looking at, which may be a tag field.

[image: Screenshot 2023-05-05 at 9 53 41 AM] https://user-images.githubusercontent.com/5910297/236493287-a3808097-951f-475d-8f91-d965942946dd.png

— Reply to this email directly, view it on GitHub https://github.com/DiscipleTools/disciple-tools-web-components/pull/60#issuecomment-1536382574, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKQ7S72NCOJNOEVE7ABRPXTXEUIELANCNFSM6AAAAAAXVOYDOE . You are receiving this because you were mentioned.Message ID: @.*** com>

incraigulous commented 1 year ago

Correct, but I'm using it inside of a magic link for a different purpose. I just tried the tag field with allowAdd and I don't get an Add button.

Edit: I got it to work

incraigulous commented 1 year ago

Do you think we should just move the allowAdd functionality upstream to the multiselect field? It seems like it is something it would make sense for the multiselect to allow. Alternatively, I could just use the dt-tags field.

incraigulous commented 1 year ago

I just tried the tags field. It posts objects, so it doesn't work in a standard non-ajax form context.

Screenshot 2023-05-05 at 10 27 19 AM

I would have to extend it to use it. I could do that, but it makes sense for the multiselect field to have that functionality, in my opinion. We could have the multi-select post a comma-separated string list as mine is, and the tag field can keep the current functionality.

Alternatively, we could add an option to format the dt-tags value as a string.

cairocoder01 commented 1 year ago

Ah, I see what your problem is. I've actually thought about changing the value of the tags field to just be an array of strings because the full object array isn't really necessary. I've kinda regretted how I did that. If that were the case and it posted an array of strings, would that work for your use case? If not, I'm sure we could move allowAdd up to multi-select. The multi-select just isn't setup to pull in the list of options via ajax, so that is the main difference between the two fields.

cairocoder01 commented 1 year ago

This also brings up another issue with how we set the form value that is used when submitting the form in a standard way like that. For fields with complex value types (like this and the connection field), we're going to get that kind of a problem. I think we might need to handle complex values in the DtFormBase._setFormValue(value) method. Maybe we JSON stringify, or we just simplify it if possible. It will take some further testing.

cairocoder01 commented 1 year ago

I just published version 0.3.4 of the web components that handles complex values properly. When posted via standard form, it uses the formdata object notation that should be widely supported. It still provides the value by JSON if you access the element directly in JS and use el.value.

image

I'm still happy to convert the value of dt-tags to be a flat list as well if you prefer. It is on my backlog of things I want to fix anyways.

incraigulous commented 1 year ago

Much appreciated! That will solve my immediate problem. Do we want to move addNew upstream to the multi-select field or just leave the mutl-select field as is?

cairocoder01 commented 1 year ago

Glad that works for you. I think we leave multi-select as-is. It currently represents the multi-select field as defined within DT's custom fields, which is a statically-defined list that can't be added to. The tags field is the slight adjustment to it that has a dynamic options list and allows adding new items. I think it makes sense to leave them as-is, if that works for you. I'm currently working on the change to the dt-tags value so that it is a flat list and you can specify it as <dt-tags value="['Value 1','Value2']" />. If that's good for you, I think you can close this PR.

cairocoder01 commented 1 year ago

I'm closing this as I've also just released v0.4.0 that changes dt-tags to take a value that is a flat list of strings. This makes it match the functionality of multi-select while additionally having the addNew option. It is a bit of a breaking change due to the format of the value attribute, so be aware of that.