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

script#edit don't allow user to remove last option #3844

Merged
merged 6 commits into from
Oct 14, 2024
Merged

Conversation

ashton22305
Copy link
Contributor

Fixes #3050

Disables the remove button for the last option.

Note that this does not actually fix the attribute, just removes the ability for the user to click the remove button on the last option.

If we want to fix the option, it might make sense to allow the user to unfix the attribute by adding an item another item instead of disabling all of them. Otherwise, if the the attribute was fixed when there is only one option left, the "Fixed" checkbox would need to be unchecked and some arbitrary second option would need to be enabled.

@johrstrom
Copy link
Contributor

I feel like there's a ticket for being unable to remove script & cluster. Like you can't ever remove these 2 things, ever. The system just can't do anything if those 2 aren't supplied.

@johrstrom
Copy link
Contributor

🤦‍♂️ you linked the ticket I was thinking about, but even in that ticket (that I wrote lol) I didn't mention these 2 fields script and cluster.

Looking now I see that script and cluster don't have delete buttons, so that's good. #3050 may be complete then because you can't remove those 2?

@ashton22305
Copy link
Contributor Author

Maybe I misunderstood exactly what the ticket was saying? In an individual dropdown select (i.e. script), currently the user will get a JS alert if the remove button on the last option is clicked. This PR instead just disabled that button.

@johrstrom
Copy link
Contributor

Maybe I misunderstood exactly what the ticket was saying? In an individual dropdown select (i.e. script), currently the user will get a JS alert if the remove button on the last option is clicked. This PR instead just disabled that button.

Sorry, it's me who's mis-understaing. Yes I think you're correct, and I'll take a look at this sometime soon (I'm a bit snowed in with 3.1 and some cardinal stuff).

@@ -34,7 +35,7 @@

<button class="btn btn-warning float-end" type="button" id="<%= remove_id %>"
data-select-toggler="remove" data-select-id="<%= field_id %>"
<%= disabled ? 'disabled="true"'.html_safe : nil %> >
<%= disabled || last_option ? 'disabled="true"'.html_safe : nil %> >
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid using html_safe. Here it's just text so it should be fine. Not sure why the original code used it, I must not have caught it then.

@johrstrom
Copy link
Contributor

This works well for me and indeed I think I like the UX better that what we have now (with the alert). I think I just have one little thing with the html_safe in the partial.

@johrstrom johrstrom merged commit 3a83dfd into master Oct 14, 2024
26 checks passed
@johrstrom johrstrom deleted the fix-last-option branch October 14, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

script#edit shouldn't allow users to remove all select options.
3 participants