Skip to content

InputSelectEnum #34823

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

Closed
GravlLift opened this issue Jul 28, 2021 · 11 comments
Closed

InputSelectEnum #34823

GravlLift opened this issue Jul 28, 2021 · 11 comments
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future

Comments

@GravlLift
Copy link

GravlLift commented Jul 28, 2021

Summary

A new Blazor form component that automatically lists enum values that an InputSelect can receive.

Motivation and goals

I often find myself writing this same InputSelect code over and over:

<InputSelect @bind-Value="model.SomeEnumVal">
    @foreach(var enumVal in Enum.GetValues<SomeEnumVal>())
    {
        <option value=@enumVal>@enumVal</option>
    }
</InputSelect>

A separate InputSelectEnum<TValue> component that removes this boilerplate could be useful, imo.

// Identical output to above
<InputSelectEnum @bind-Value="model.SomeEnumVal" />

I would write this privately for myself, copying the existing InputSelect code and tweaking it. The only changes from the existing code is a modification of the BuildRenderTree method to generate options instead of displaying child content.

However, certain pieces of the InputSelect code that I'd use as a baseline are internal, such as the InputExtensions.TryParseSelectableValueFromString method and InputBase.FieldIdentifier used for value binding and validation messages, respectively.

In scope

  • Generating a blazor select with all possible enum values
  • Support for nullable enum as a valid TValue
  • Input parameter for changing how enum values are displayed, in the form of Func<TValue, string>. When this parameter is absent, display using System.CompnentModel.DataAnnotations.DisplayAttribute.Name if available, then ToString()

Out of scope

  • Filtering specific enum values from the options

Risks / unknowns

If we wish to support nullable enums, then using the generic constraint where TValue : Enum is not a valid path. Therefore, type validity will need to be enforced at runtime.

Examples

An existing example of another developer implementing this component can be found here. That particular implementation may have been using the value binding code from this repo as it existed but it now differs from the code we find in main, and thus results in an inconsistent experience alongside the equivalent InputSelect.

@GravlLift GravlLift added the design-proposal This issue represents a design proposal for a different issue, linked in the description label Jul 28, 2021
@mkArtakMSFT mkArtakMSFT added the area-blazor Includes: Blazor, Razor Components label Jul 28, 2021
@campersau
Copy link
Contributor

Should [Flags] enums be rendered as <select multiple> ?
Should it support attributes like System.ComponentModel.DataAnnotations.DisplayAttribute to display <option value=@enumVal>@enumDisplayName</option> when no Func<TValue, string> is provided?

@GravlLift
Copy link
Author

Should [Flags] enums be rendered as <select multiple> ?

I would think we'd stick to the currently established convention of InputSelect where select multiple is determined by TValue being an array.

public class InputSelect<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TValue> : InputBase<TValue>
{
private readonly bool _isMultipleSelect;
/// <summary>
/// Constructs an instance of <see cref="InputSelect{TValue}"/>.
/// </summary>
public InputSelect()
{
_isMultipleSelect = typeof(TValue).IsArray;
}

Should it support attributes like System.ComponentModel.DataAnnotations.DisplayAttribute to display <option value=@enumVal>@enumDisplayName</option> when no Func<TValue, string> is provided?

That's a good idea, and seems to be consistent with the behavior of Microsoft.AspNetCore.Mvc.Rendering.HtmlHelper.GetEnumSelectList.

@mkArtakMSFT
Copy link
Contributor

Thanks for contacting us.
The proposed solution for such a component that you have above looks great and we would like this to be something application developers to write on their own - or provide such a component to the community. We won't build one into the framework.

@campersau
Copy link
Contributor

@DamianEdwards mentioned something like this here #33950 (comment)

@GravlLift
Copy link
Author

@mkArtakMSFT any chance of Microsoft.AspNetCore.Components.Forms.InputExtensions getting moved from internal to public? That being inaccessible to the community really the only reason that I'm proposing building it into the framework at all.

@DamianEdwards
Copy link
Member

I personally think these kinds of things should be in the framework by default. Punting to the community every time there's an opportunity to provide clear "customer delight" for very common scenarios is misguided IMO. In this case I'd go further and wouldn't make it a discrete component but rather make InputSelect support it.

@mkArtakMSFT mkArtakMSFT added this to the Backlog milestone Aug 2, 2021
@ghost
Copy link

ghost commented Aug 2, 2021

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mkArtakMSFT mkArtakMSFT reopened this Aug 2, 2021
@mkArtakMSFT mkArtakMSFT added enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future labels Aug 2, 2021
@TanayParikh TanayParikh modified the milestones: Backlog, .NET 7 Planning Jun 17, 2022
@ghost
Copy link

ghost commented Jun 17, 2022

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

@SteveSandersonMS
Copy link
Member

While I see there are definitely cases where this would help, it does bias towards quick prototyping over cases where you're trying to make a well-polished result, because:

  • It's not clear how you'd get entries to come out in a desired order. The display order may need to vary from the enum value order to make the most sense to users.
  • You'd lose the ability to filter values in and out based on other conditions, and they're all included unconditionally. It's pretty dangerous, since a developer working on an unrelated app feature may add a new enum entry and have no idea that they are about to change the UI elsewhere on the site and introduce invalid options.
  • If you're going to the trouble of defining a DisplayText callback, it's arguably just as easy to hardcode a set of <option> tags since that also gives an equally convenient way to define the display texts
  • We don't generally support DisplayName, and doing so here might set incorrect expectations about what other framework components should do
  • Simply having a @foreach over Enum.GetValues<SomeEnumVal>() is not at all difficult anyway

@ghost
Copy link

ghost commented Jun 20, 2022

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, BlazorPlanning Nov 5, 2023
@mkArtakMSFT
Copy link
Contributor

Closing based on @SteveSandersonMS 's last response, given that we've received no further objections to that response.

@mkArtakMSFT mkArtakMSFT closed this as not planned Won't fix, can't repro, duplicate, stale Nov 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components design-proposal This issue represents a design proposal for a different issue, linked in the description enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-builtin-components Features related to the built in components we ship or could ship in the future
Projects
None yet
Development

No branches or pull requests

7 participants