Skip to content
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

Adds select placeholder feat #6854

Conversation

brianchristopherbrady
Copy link

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

  • I have included a change request file using $ yarn change
  • 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

⏭ Next Steps

Adds tests for placeholder feature. The

@brianchristopherbrady brianchristopherbrady changed the title User/brianbrady/select placeholder Adds select placeholder feat Nov 2, 2023
@radium-v
Copy link
Collaborator

radium-v commented Nov 3, 2023

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.

Copy link
Collaborator

@radium-v radium-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding for the addition of test fixtures.

@radium-v
Copy link
Collaborator

radium-v commented Nov 3, 2023

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
Copy link
Author

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
Copy link
Author

brianchristopherbrady commented Dec 19, 2023

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.

@radium-v radium-v force-pushed the user/brianbrady/select-placeholder branch from fa4f560 to 4055e1c Compare January 6, 2024 00:24
@bheston
Copy link
Collaborator

bheston commented Jan 9, 2024

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
Copy link
Collaborator

radium-v commented Jan 9, 2024

@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
Copy link
Collaborator

janechu commented Jun 10, 2024

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

@janechu janechu closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants