microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.28k stars 595 forks source link

Adds select placeholder feat #6854

Closed brianchristopherbrady closed 5 months ago

brianchristopherbrady commented 1 year ago

Select Placeholder

Description

This PR adds placeholder functionality to the FASTSelect component. The implementation aligns with how native HTML select elements handle placeholder text. Specifically, a hidden and disabled option element is dynamically rendered within the select menu to display the placeholder text.

Features

Adds a placeholder attribute to the FASTSelect component. Dynamically renders a hidden and disabled option containing the placeholder text. If the placeholder attribute is undefined or empty, the placeholder option will not be rendered.

Benefits

Provides a standardized way to display placeholder text in FASTSelect, similar to native HTML select elements. Enhances the usability of the FASTSelect component by indicating the kind of selection the user should make.

🎫 Issues

[<!---

πŸ“‘ Test Plan

βœ… Checklist

General

Component-specific

⏭ Next Steps

Adds tests for placeholder feature. The

radium-v commented 1 year ago

It would be good to see some tests around required, to confirm that it fails to submit an empty value when the placeholder item is still selected during form submit.

radium-v commented 1 year ago

Adding a placeholder while in non-collapsible mode (multiple or with a size) still creates the shadow placeholder option. Should this be accounted for or ignored?

brianchristopherbrady commented 10 months ago

It would be good to see some tests around required, to confirm that it fails to submit an empty value when the placeholder item is still selected during form submit.

@radium-v

From what I can gather of the workspace it doesn't appear that there isn't any specific handling of required inputs in relation to form submission. The codebase does not seem to contain logic that prevents form submission when a required control is missing a value? Please correct me if I am wrong here.

The tests I've come across, such as the one for the fast-checkbox component, are primarily checking the validity of the control's value rather than its effect on form submission. Here's an example from the checkbox tests:

test("should be invalid when unchecked", async () => {
    await root.evaluate(node => {
        node.innerHTML = /* html */ `
            <form>
                <fast-checkbox required></fast-checkbox>
            </form>
        `;
    });

    expect(
        await element.evaluate((node: FASTCheckbox) => node.validity.valueMissing)
    ).toBe(true);
});

I've added similar tests to the Select tests.

Thoughts?

brianchristopherbrady commented 10 months ago

Adding a placeholder while in non-collapsible mode (multiple or with a size) still creates the shadow placeholder option. Should this be accounted for or ignored?

@radium-v

I added another conditional to the template that checks if the Select is collapsible and if so then render the placeholder option otherwise do not.

I've added a couple tests for this behavior as well.

bheston commented 10 months ago

I'm glad to see this feature, though I wonder if there's benefit in handling it the way it must be done in a native element, or if the native method represents more of a workaround due to lack of native placeholder capability. In the case of input fields, we're wrapping native inputs so we get placeholder capability for free. In this component I wonder if it would be preferable to use a native input for placeholder also.

From a structural perspective we've been trying to increase coherence of the component templates. One improvement from this is for consistent styling application. For instance, any consumers who want to override placeholder text styling would need to provide different treatments for inputs and the select.

radium-v commented 10 months ago

@bheston the templates for Combobox and Select are unique to accomodate differences in their accessibility requirements. If we're considering major changes, we could theoretically roll Select and Combobox into one component, but doing this would likely eliminate multi-selection from Select (Combobox doesn't have mult-selection). Listbox could take its place for that feature.

These would be massive changes to the three components, but it could be a way forward if authors agree that this would clarify and simplify their usage going forward. I can make an RFC to discuss it.

janechu commented 5 months ago

Closing this, due to #6951 we are only putting in fixes or critical features.