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

feat: add search box spec #5238

Closed
wants to merge 7 commits into from
Closed

feat: add search box spec #5238

wants to merge 7 commits into from

Conversation

eljefe223
Copy link
Contributor

Pull Request

πŸ“– Description

Adds search box spec

🎫 Issues

Addresses: #4967

πŸ‘©β€πŸ’» Reviewer Notes

πŸ“‘ 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

@eljefe223 eljefe223 force-pushed the users/jes/search-spec branch from d370fca to 96c2924 Compare October 5, 2021 16:11
specs/search/search.md Outdated Show resolved Hide resolved
specs/search/search.md Outdated Show resolved Hide resolved
specs/search/search.md Outdated Show resolved Hide resolved
specs/search/search.md Outdated Show resolved Hide resolved

```HTML
<!-- shadow root -->
<label part="label"><slot></slot></label>
Copy link
Member

Choose a reason for hiding this comment

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

I think we have an issue out for whether or not we should include labels on form elements - would be good to look at that prior to finalizing this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TextField and TextArea both have them. Even though its not a common design pattern there may be cases where we want a label.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'm aware of the prior implementation - I think there is a larger discussion to have here though as some form elements include this and others do not. One of the primary issues we've seen is associating across the light and shadow DOM. There are a couple proposals out there being made, but nothing exists now to solve the problem. Right now we aren't consistent and there are issues with the current approach from an accessibility standpoint. The question I think we have to ask is what is our best path forward as of now with regards to components including labels. How far out is any solution? Is the label part of the control or is it better off as part of a "form-field" component? If "it depends", is there a clear line for when we would and would not include a label as part of a component?

Copy link
Member

Choose a reason for hiding this comment

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

Here's an example of something recently proposed for a platform enhancement to help with this, but again - it doesn't yet exist. Regardless of what is coming, I think the question above in regard to the components themselves likely needs to be answered for what our path forward is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whatever we do we should be consistent, if its possible to separate the label in the form of of a form-field component that seems like a more flexible approach.

specs/search/search.md Outdated Show resolved Hide resolved
- default - the label content
- before-content - often times a glyph, icon, or button precedes input
- after-content - often times a glyph, icon, or button follows the input
- clear-content - often times a glyph, icon, or button follows that clears the input field
Copy link
Member

Choose a reason for hiding this comment

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

could this just be slotted in end or does it require it's own slot?

Copy link
Contributor Author

@eljefe223 eljefe223 Oct 11, 2021

Choose a reason for hiding this comment

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

I think we do. What about cases where we would want a button to the right of the input that executes the search?

image

Copy link
Member

Choose a reason for hiding this comment

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

We aren't limited to one slotted element per slot, I'm primarily curious of how common the clear selection is and how difficult it is to wire up something without it's own slot. Is it prohibitive or just preferred because it seems easier? There's no right or wrong answer here, but it seems like we're providing a pretty specific slot for a specific implementation. More research to point to this being consistent enough that it should have a unique and custom slot just for this behavior. If we don't need to wire up any behavior to the clear button by default and there's nothing prohibitive, I'm just curious about how much value the unique slot provides.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The clear button is the defaulted in the react fluent controls. It is also a common pattern across many search box examples. Perhaps we just provide a slot for the glyph inside the button, The functionality is directly tied to the input so slotted the whole button makes less sense.

OR we just slot in any number of after content and the clear button can be one of them. It would just be on the author to wire up the clear functionality.

Copy link
Member

Choose a reason for hiding this comment

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

I don't particularly care about specific implementations such as how the Fluent React controls are built out - I'm primarily concerned right now with gather requirements and then determining what gets first-party support. Our goal should be to enable a majority of use cases, I don't think that always requires us to prioritize all use cases with first-party API support.

If the above is under consideration (baking a button into search), I think we need more research and examples of that being the most common use case. I think we'll find that it's a use case, but not necessarily the most common. The next thing that comes to my mind are, "what are the implications of baking a control in like that"? We don't follow that pattern anywhere else, so it would be new. What possible draw backs exist? I have a few questions that immediately come to mind - how do I bind click events, keyboard events, etc to the default button that are unique to my implementation? How do we handle scenarios where a user wants to add telemetry to the default button? Defaulting interactive controls in the shadow DOM isn't a pattern we typically have and I think we need more research than what I see here to determine that as the right and intuitive course of action.

Copy link
Contributor

Choose a reason for hiding this comment

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

Possible assumptions about a clear feature:

  • Always related to presentation
  • "Clear" is always presented as a glyph
  • The slotted element is focusable (A11y implications)
  • Search specifically needs clear as opposed to all input fields.
  • Always a web search

specs/search/search.md Outdated Show resolved Hide resolved
specs/search/search.md Show resolved Hide resolved
specs/search/search.md Show resolved Hide resolved
- default - the label content
- before-content - often times a glyph, icon, or button precedes input
- after-content - often times a glyph, icon, or button follows the input
- clear-content - often times a glyph, icon, or button follows that clears the input field
Copy link
Contributor

Choose a reason for hiding this comment

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

Possible assumptions about a clear feature:

  • Always related to presentation
  • "Clear" is always presented as a glyph
  • The slotted element is focusable (A11y implications)
  • Search specifically needs clear as opposed to all input fields.
  • Always a web search

@eljefe223 eljefe223 force-pushed the users/jes/search-spec branch from 7e4704b to ae3c36a Compare October 13, 2021 16:05
@eljefe223 eljefe223 force-pushed the users/jes/search-spec branch from ae3c36a to 4a6a329 Compare October 13, 2021 18:54
@EisenbergEffect
Copy link
Contributor

Can this be closed?

@EisenbergEffect
Copy link
Contributor

Seems that we already merged the component. Does this spec match what was implemented and can we merge it?

- filled

*Events*
- `change: CustomEvent`
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably have both change and input events here.

@chrisdholt
Copy link
Member

Seems that we already merged the component. Does this spec match what was implemented and can we merge it?

@eljefe223 can you make sure this is up to date and reflects where we currently are? Ideally we ensure these are merged before the component work is done :)

@awentzel awentzel changed the title feat: [draft] add search box spec feat: add search box spec Mar 8, 2022
- [Ant Design](https://ant.design/components/input/)
- [Windows (UWP)](https://docs.microsoft.com/en-us/windows/apps/design/controls/auto-suggest-box)

---
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: --- seems out of place or unnecessary here.

@chrisdholt chrisdholt closed this Nov 16, 2023
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.

7 participants