fast icon indicating copy to clipboard operation
fast copied to clipboard

Adds select placeholder feat

Open brianchristopherbrady opened this issue 1 year ago • 6 comments

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

[<!---

  • List and link relevant issues here. -->](https://github.com/microsoft/fast/issues/6853)

📑 Test Plan

✅ Checklist

General

  • [ ] I have included a change request file using $ yarn change
  • [x] I have added tests for my changes.
  • [x] I have tested my changes.
  • [x] I have updated the project documentation to reflect my changes.
  • [x] I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

⏭ Next Steps

Adds tests for placeholder feature. The

brianchristopherbrady avatar Nov 02 '23 00:11 brianchristopherbrady

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 avatar Nov 03 '23 17:11 radium-v

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 avatar Nov 03 '23 17:11 radium-v

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 avatar Dec 19 '23 20:12 brianchristopherbrady

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.

brianchristopherbrady avatar Dec 19 '23 21:12 brianchristopherbrady

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.

bheston avatar Jan 09 '24 02:01 bheston

@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.

radium-v avatar Jan 09 '24 21:01 radium-v

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

janechu avatar Jun 10 '24 21:06 janechu