Skip to content

InputTime component with form support #26324

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
wants to merge 1 commit into from
Closed

InputTime component with form support #26324

wants to merge 1 commit into from

Conversation

MarvinKlein1508
Copy link

This adds an InputTime component which can be used to enter only time values and not just date values.

@ghost ghost added the area-blazor Includes: Blazor, Razor Components label Sep 25, 2020
@dnfadmin
Copy link

dnfadmin commented Sep 25, 2020

CLA assistant check
All CLA requirements met.

@SteveSandersonMS
Copy link
Member

Thanks for contributing this, @MarvinKlein1508!

I notice that the implementation is almost identical to the one for InputDate. This makes me wonder if we really should have a separate component, or whether we could just slightly generalise InputDate to handle times. This would make sense since the .NET value type is still DateTime or DateTimeOffset, so the value you are inputting is still a date in the .NET sense even if you only care about the time portion of that date.

To achieve this, I think we could:

  1. Modify the order in which InputDate renders its attributes so that it renders the type attribute before the AdditionalAttributes. This will make it possible for developers to override the type, e.g., <InputDate type="time" ... />
  2. Add an extra optional parameter named ValueFormat that defaults to yyyy-MM-dd. Developers who want the "time" behavior can do <InputDate type="time" ValueFormat="HH:mm" ... /> (or HH:mm:ss).

The other benefit of this approach is that we'd get support for type="datetime-local" for free. The developer would just pass a different "type" and "Format" parameter. Likewise with any other similar HTML date/time pickers that get invented in the future.

What do you think?

Also, for to be able to accept the PR, would you be able to add corresponding E2E tests? Please see existing E2E tests in BindCasesComponent.razor for examples of how to add this.

@MarvinKlein1508
Copy link
Author

Hi @SteveSandersonMS

thanks for your reply. I was thinking the exact same thing before I created the component. My first approach was addding an enum to InputDate which specifies the control as Date, Time or DateTimeLocal. However I could find a solution to get the correct format syntax within the static methods.

I don't think we should provide the possibility to set a specific format. As you previously mentioned here #15400 (comment) the format is always the same. Only the displaying format vary from the browser and culture of the user.

For timecomponent the format is always HH:mm:ss.
For datetime-local it is always yyyy-MM-ddThh:mm

I as a user would also find it confusing if an InputDate Component could also represent time. So I would adopt this component to InputDateTime. This would also show me that I can use other formats here. But to keep existing projects running, I had created an InputTime component from it.

I will provide you an E2E test, once I get the solution compiling. Still haven't figured out yet how to get all dependencies. I have followed this guidelines step by step but something seems to be missing on my side.
https://github.com/dotnet/aspnetcore/blob/master/docs/BuildFromSource.md

-Marvin

@Pilchie Pilchie added the community-contribution Indicates that the PR has been added by a community member label Oct 1, 2020
@uabarahona
Copy link
Contributor

Just to keep track, this seems to be realated with #12376

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Dec 1, 2020

For timecomponent the format is always HH:mm:ss.
For datetime-local it is always yyyy-MM-ddThh:mm

@MarvinKlein1508 We've been considering this in more detail and think there's a simpler design based on your observations. Since the type attribute is well defined in web standards, and completely determines the value format independently of any globalization concerns, we can just make InputDate sensitive to the type attribute and vary its value format based on that. This eliminates the need for any new Blazor-specific APIs, and just makes it follow from common web standards.

I know you already raised a concern about the naming, i.e., that it would make more sense for it to be called InputDateTime considering its generality. However this has already shipped and it's not a strong enough reason to either rename or alias it. We don't think developers will have any real problem with <InputDate type="time" @bind-Value="myDateTimeField" /> or <InputDate type="datetime-local" @bind-Value="myDateTimeField" />.

So to proceed with this, our proposal is:

  1. To make InputDate recognize supplied time attributes and use that as the time attribute it outputs
  2. In the case where the time attribute value is one of a known set (date, time, datetime-local, any others?), the internal value format should reflect that. If it's unknown, throw an exception.
  3. Add E2E tests

Does that sound feasible?

@MarvinKlein1508
Copy link
Author

MarvinKlein1508 commented Dec 1, 2020

@SteveSandersonMS
yes this makes much more sense to prevent duplicate code.

any others?

Not sure but there is also an input type for week

I haven't used this one before so I am not sure whether it is compatible with datetime.

Add E2E tests

Where should I put them in this repository? Can you link an E2E test for another simple InputComponent?

@SteveSandersonMS
Copy link
Member

Existing E2E tests for the Input* components are in FormsTest.cs (in the E2ETest project). The corresponding sample code being tested is in BasicTestApp within the FormsTest subdirectory.

@SteveSandersonMS SteveSandersonMS added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Dec 1, 2020
Base automatically changed from master to main January 22, 2021 01:33
@ghost
Copy link

ghost commented Feb 18, 2021

Hi @MarvinKlein1508.
It seems you haven't touched this PR for the last two weeks. To avoid accumulating old PRs, we're marking it as stale. As a result, it will be closed if no further activity occurs within 4 days of this comment. You can learn more about our Issue Management Policies here.

@ghost ghost added the stale Indicates a stale issue. These issues will be closed automatically soon. label Feb 18, 2021
@ghost ghost closed this Feb 24, 2021
@MarvinKlein1508
Copy link
Author

@SteveSandersonMS

Hi Steve,
I came across the need of this component again recently. So I gave it a new try. Unfortunately I don't get the aspnetcore repo to compile with my settings so I created the component within a custom razor class library. I came across this solution:

/// <summary>
    /// An input component for editing date values.
    /// Supported types are <see cref="DateTime"/> and <see cref="DateTimeOffset"/>.
    /// </summary>
    public class InputDate<TValue> : InputBase<TValue>
    {
        /// <summary>
        /// Gets or sets the error message used when displaying an a parsing error.
        /// </summary>
        [Parameter] public string ParsingErrorMessage { get; set; } = "The {0} field must be a date.";
        
        /// <summary>
        /// Gets or sets the <see cref="InputDateFormat"/> for the input.
        /// </summary>
        [Parameter] public InputDateFormat Format { get; set; } = InputDateFormat.Date;

        /// <summary>
        /// Gets or sets the associated <see cref="ElementReference"/>.
        /// <para>
        /// May be <see langword="null"/> if accessed before the component is rendered.
        /// </para>
        /// </summary>
        [DisallowNull] public ElementReference? Element { get; protected set; }

        /// <summary>
        /// Gets the corresponding input type for the provided <see cref="Format"/>
        /// </summary>
        protected string InputType => Format switch
        {
            InputDateFormat.Date => "date",
            InputDateFormat.DateTimeLocal => "datetime-local",
            InputDateFormat.Time => "time",
            _ => throw new InvalidOperationException($"The format '{Format} is not supported'")
        };

        /// <summary>
        /// Gets the corresponding input format for the provided <see cref="Format"/>
        /// </summary>
        protected string InputFormat => Format switch
        {
            InputDateFormat.Date => "yyyy-MM-dd",
            InputDateFormat.DateTimeLocal => "yyyy-MM-ddThh:mm",
            InputDateFormat.Time => "hh:mm:ss",
            _ => throw new InvalidOperationException($"The format '{Format} is not supported'")
        };

        /// <inheritdoc />
        protected override void BuildRenderTree(RenderTreeBuilder builder)
        {
            builder.OpenElement(0, "input");
            builder.AddMultipleAttributes(1, AdditionalAttributes);
            builder.AddAttribute(2, "type", InputType);
            builder.AddAttribute(3, "class", CssClass);
            builder.AddAttribute(4, "value", BindConverter.FormatValue(CurrentValueAsString));
            builder.AddAttribute(5, "onchange", EventCallback.Factory.CreateBinder<string?>(this, __value => CurrentValueAsString = __value, CurrentValueAsString));
            builder.AddElementReferenceCapture(6, __inputReference => Element = __inputReference);
            builder.CloseElement();
        }

        /// <inheritdoc />
        protected override string FormatValueAsString(TValue? value)
        {
            switch (value)
            {
                case DateTime dateTimeValue:
                    return BindConverter.FormatValue(dateTimeValue, InputFormat, CultureInfo.InvariantCulture);
                case DateTimeOffset dateTimeOffsetValue:
                    return BindConverter.FormatValue(dateTimeOffsetValue, InputFormat, CultureInfo.InvariantCulture);
                default:
                    return string.Empty; // Handles null for Nullable<DateTime>, etc.
            }
        }

        /// <inheritdoc />
        protected override bool TryParseValueFromString(string? value, [MaybeNullWhen(false)] out TValue result, [NotNullWhen(false)] out string? validationErrorMessage)
        {
            // Unwrap nullable types. We don't have to deal with receiving empty values for nullable
            // types here, because the underlying InputBase already covers that.
            var targetType = Nullable.GetUnderlyingType(typeof(TValue)) ?? typeof(TValue);

            bool success;
            if (targetType == typeof(DateTime))
            {
                success = TryParseDateTime(value, InputFormat, out result);
            }
            else if (targetType == typeof(DateTimeOffset))
            {
                success = TryParseDateTimeOffset(value, InputFormat, out result);
            }
            else
            {
                throw new InvalidOperationException($"The type '{targetType}' is not a supported date type.");
            }

            if (success)
            {
                Debug.Assert(result != null);
                validationErrorMessage = null;
                return true;
            }
            else
            {
                validationErrorMessage = string.Format(CultureInfo.InvariantCulture, ParsingErrorMessage, DisplayName ?? FieldIdentifier.FieldName);
                return false;
            }
        }

        private static bool TryParseDateTime(string? value, string format, [MaybeNullWhen(false)] out TValue result)
        {
            var success = BindConverter.TryConvertToDateTime(value, CultureInfo.InvariantCulture, format, out var parsedValue);
            if (success)
            {
                result = (TValue)(object)parsedValue;
                return true;
            }
            else
            {
                result = default;
                return false;
            }
        }

        private static bool TryParseDateTimeOffset(string? value, string format, [MaybeNullWhen(false)] out TValue result)
        {
            var success = BindConverter.TryConvertToDateTimeOffset(value, CultureInfo.InvariantCulture, format, out var parsedValue);
            if (success)
            {
                result = (TValue)(object)parsedValue;
                return true;
            }
            else
            {
                result = default;
                return false;
            }
        }
    }

As you can see I've added two get only properties:
InputType and InputFormat

and one new property as parameter called Format. Format takes an enum of type InputDateFormat which is defined as:

/// <summary>
    /// Defines the types of an <see cref="InputDate{TValue}"/> component.
    /// </summary>
    public enum InputDateFormat
    {
        Date,
        DateTimeLocal,
        Time
    }

I also created a short test project to see if everythings works. It turns out that type date works fine. datetime-local and time doesn't work fully yet because they need to get another CultureInfo.

For example: the validation of time works until 12 hours and 59 minutes. In europe we enter the time until 23 hours and 59 minutes. But I am not sure how this is done correctly (I haven't worked as much with CultureInfo because I only needed to optimise software for our own purposes yet). But I am pretty sure you will know how to change this.

@ghost
Copy link

ghost commented Jul 22, 2021

Hi @MarvinKlein1508. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-blazor Includes: Blazor, Razor Components community-contribution Indicates that the PR has been added by a community member pr: pending author input For automation. Specifically separate from Needs: Author Feedback stale Indicates a stale issue. These issues will be closed automatically soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants