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

Selectlist select none #677

Conversation

ryanmitchell
Copy link
Member

Initial idea - basically make placeholder always there (obviously need to add lang string for default for this but havent done that yet) and change it to be 'select none'.

Then made the various events toggle between select all and select none.

@sampoyigi
Copy link
Member

I still don't see how this is any different from just defining the placeholder on the field config?

I don't think we need both Select All and Select None options at all times when we can achieve the same behaviour with just the Select all option.

I see what you mean - you need to select the placeholder value though - if you select nothing is comes through blank? Doesnt seem intuitive to me. I'll look into the JS library and see if there is a way of always passing a value.

What about adding an empty option (if not already present) to the list when nothing is selected (when Select All is deselected and when the field is loaded with no selected options)

@ryanmitchell
Copy link
Member Author

With the last commits select none is the placeholder/empty option? So is the same as adding an empty option. Maybe I’m misunderstanding what you mean.

@sampoyigi
Copy link
Member

Yea I mean adding it in the background like a hidden input or disabled empty option?

@ryanmitchell
Copy link
Member Author

Updated, let me know what you think

@sampoyigi
Copy link
Member

Ok, I think we're actually changing the normal behaviour of the select field. The select field only appears in the POST data when it has selected options just like checked inputs.

Forcing an empty string array when no option is selected would actually break components using the selectlist field.

If you take the FeaturedMenus component for example. With this PR implemented when you save the component with no menu items selected, you get an empty string array which might break the frontend.

@ryanmitchell
Copy link
Member Author

ryanmitchell commented Jan 29, 2021 via email

@sampoyigi
Copy link
Member

Yup, that would work to force an empty option value... But then you can do the same with the placeholder option.. I think I still don't fully understand the issue here.

@ryanmitchell
Copy link
Member Author

That’s ok I’ll try and explain.

Placeholder works as part of the fix but say I set the value to ‘none’, then it still keeps everything else selected when chosen. So you can have none and other values. From a user point of view that feels strange.

What about a config option ‘selectEmptyClearsSelection’ - which then uses the logic here - so in my example selecting none would clear all other values. And none is added via the placeholder if specified or the hidden field otherwise.

@sampoyigi
Copy link
Member

Ahh, so would attach some kind of event on the placeholder option so that its the only selected option when chosen? The hidden input just doesn't feel right.

@ryanmitchell
Copy link
Member Author

Yes - if you look at the removed code in this commit, thats basically what it was doing originally :)
90600a8

We just hide it behind a config option instead of making it default?

@sampoyigi
Copy link
Member

Yea didn't understand the issue then lol.

So if the only issue is not being able to select none when a placeholder is defined, then maybe revert to 90600a8 and we won't need the hidden input or a config option as all we need to fix is deselecting the other options when the placeholder is selected.

@ryanmitchell
Copy link
Member Author

ryanmitchell commented Jan 30, 2021 via email

@BreakSecurity
Copy link
Contributor

Should we merge this @sampoyigi if not there is an UI fix for select list zindex to cherry pick

@sampoyigi
Copy link
Member

Yea you can send another PR for the z-index fix. I'll leave this one until am sure it doesn't break something else, just like this reverted commit 874aad7.

@sampoyigi sampoyigi deleted the branch tastyigniter:stale-develop February 25, 2022 15:15
@sampoyigi sampoyigi closed this Feb 25, 2022
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.

3 participants