[AutoComplete] Only render placeholder attribute when the parameter is provided

[MarvinKlein1508]

Pull Request

📖 Description

This PR removes the conditonal check for the placeholder within FluentAutocomplete. This results in the Placeholder property only being rendered when it is actually supplied.

I've updated all unit tests to remove the empty placeholder attribute as well.

🎫 Issues

Fixes #3916

✅ Checklist

General

• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• I have added a new component
• I have added Unit Tests for my new component
• I have modified an existing component
• I have validated the Unit Tests for an existing component

[dvoituron]

This does not work: the placeholder cannot be displayed when the user has selected an item.

[MarvinKlein1508]

@dvoituron I've updated it :)

[dvoituron]

I don't understand why the unit tests must be updated

[MarvinKlein1508]

I don't understand why the unit tests must be updated

When you add the null-check to the placeholder attribute it breaks some of the existing unit tests because they expected either no placeholder at all or an empty placeholder. You can verify this by adding the null check as I did and run the old tests.

[MarvinKlein1508]

@dvoituron this happens when you add just the null check.

Always with the same error:

The old tests doesn't specify a placeholder. Therefor it rendered an empty placehodler attribute before. Which will no longer happens because we also check against null.

[dvoituron]

Yes. I know you fixed the unit tests. And these changes solve that.

But why the Placeholder is null and was empty in the dame situation. The issue is only when Multiple=False. So the current unit tests should be identical.

[MarvinKlein1508]

@dvoituron pretty simple. This is the old line:

Placeholder="@(SelectedOptions?.Any() is false ? Placeholder : string.Empty)"

This is the new line

Placeholder="@(SelectedOptions is null || SelectedOptions?.Any() is false ? Placeholder : string.Empty)"

SelectedOptions is by default null in those tests therefore it renders string.Empty as placeholder attribute. The new logic returns Placeholder property when SelectedOptions is null which is string? and defaults to null therefore no placeholder attribute will be applied at all. We should probably change this line to:

Placeholder="@(SelectedOptions is null || SelectedOptions?.Any() is false ? Placeholder : null)"

When we doing it like this then the Placeholder will only be there when a value has been provided from the user which also saves us some characters in the DOM.

[MarvinKlein1508]

I've added a commit which removes string.Empty and sets the placeholder to null to prevent it being appended to the UI when the user doesn't provide any placeholder at all. Tests have been updated for this as well.

[dvoituron]

Why to change that? This is an optimization, not a solution for this issue.

Next time, try to create 2 PR to update 2 concepts. That easier to track the goal of PRs.

[MarvinKlein1508]

@dvoituron this goes hand-in-hand together. The null check is required to fix it for single-mode. The tests needed to be adapted either way.

With 2 PRs we would change the tests twice. If you want though I can close this PR and open another one where I just update the line to the last version and update the tests.

[dvoituron]

I just approved it. What I mean is that a bug fix should only fix this problem. This makes it easier to track changes later on. If we ever find that other bugs arise.

[MarvinKlein1508]

@dvoituron yea I agree with you. But fixing this problem would require to change the unit tests as well. Therefore I would have changed it to the string.Empty version adapt the tests, just to change both parts altogether in the next PR (which would also fix the behaviour).

[dvoituron]

This is not normal; the change only applies when Multiple=False. So all current unit tests should pass.