feat: Add Search Field component #892

alimpens · 2023-12-06T11:34:04Z

To discuss:

The 'clear'-button currently uses native functionality, which only works in Chromium browsers (Chrome, Edge), not in Firefox for example. Is this functionality important enough to implement custom functionality for this?

Out of scope:

Search Field without a button (for instant search result pages, see https://onderzoek.amsterdam.nl/zoek). The design is not yet done for that.
Autosuggest
Different sizes
Tests for SearchFieldButton, will probably be part of an IconButton

…into feature/DES-507-mvp-search-field

…h box-shadow

VincentSmedinga · 2023-12-07T09:43:04Z

packages/css/src/components/search-field/README.md

Looking good! Can we add a section where we explain the design choices we made, e.g. preventing autocomplete and spellcheck, and maybe the search label we hint for the ‘Enter’ key on virtual keyboards – which I didn’t know about, very nice.

Yeah, good idea. Maybe we should wait until we reach a verdict on turning off autocomplete and spellcheck by default, or leaving it up to the dev?

packages/css/src/components/search-field/search-field.scss

VincentSmedinga · 2023-12-07T11:07:06Z

packages/react/src/SearchField/SearchField.tsx

+  ({ children, className, ...restProps }: SearchFieldProps, ref: ForwardedRef<HTMLFormElement>) => {
+    return (
+      <form role="search" {...restProps} ref={ref} className={clsx('amsterdam-search-field', className)}>
+        {children}


Why not render the input and button ourselves? This allows putting anything in a search field, the button to the left from the input, etc. What’s the advantage?

You often need access to the input itself, for autofocus, placeholder and to make it a controlled component for example. We could pass that all down from the parent component, but that would mean SearchField would need value, onChange (which would go to the child input), onSubmit (which would go to the parent component), autofocus, placeholder, etc. That's a bit messy imo. Its easier and imo more intuitive to have direct access to the input.

The button is separate because we also have SearchFields without a button, although the design for that isn't done yet.

Yes, good reasons.

VincentSmedinga · 2023-12-07T11:08:10Z

packages/react/src/SearchField/SearchFieldInput.test.tsx

Should / Can we test submitting a value?

Ah, out of scope.

No, not really, it's actually a good idea imo. We should test the controlled version of the input

VincentSmedinga · 2023-12-07T11:10:05Z

packages/react/src/SearchField/SearchFieldInput.tsx

+          <VisuallyHidden>{label}</VisuallyHidden>
+        </label>
+        <input
+          {...restProps}


Is it intentional to disallow overriding autoComplete and spellCheck?

Yes, but I wasn't completely sure about it. They're both kind of annoying when you're looking for partial text, especially in combination with an autosuggest component. We turned autoComplete off at Onderzoek, I'm not sure why we didn't turn off spellcheck. I see data.amsterdam.nl also turns autoComplete off.

I thought I stole the spellCheck part from gov.uk, but I see they don't have autoComplete and spellCheck turned off (anymore).

We could leave it up to the devs?

I think setting them to off initially is actually the best MVP approach – setting them on indeed has UX side-effects.

In general, we should be explicit in making HTML-attributes overridable or not – i.e. specifying them before or after {...restProps}. I made a note to discuss later.

This comment can be resolved.

packages/react/src/SearchField/SearchFieldInput.tsx

VincentSmedinga · 2023-12-07T11:14:01Z

proprietary/tokens/src/components/amsterdam/search-field.tokens.json

+        "outline-offset": { "value": "{amsterdam.focus.outline-offset}" },
+        "cancel-button": {
+          "background-image": {
+            "value": "url(\"data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32'><path fill='%23004699' fill-rule='evenodd' d='M29.333 5.47 26.53 2.668 16 13.187 5.47 2.666 2.668 5.47 13.187 16 2.666 26.53l2.804 2.803L16 18.813l10.53 10.52 2.803-2.804L18.813 16z'/></svg>\")"


Sweet. Move the path into an icon token later?

I'm not sure what you mean, do you want to create tokens for all icons?

I can imagine using background icons more often. We could have an icons.tokens.json containing the value for path.d, or the entire svg, to allow reusing it later. We should extend the build step creating React components to also create these tokens.

Just an idea. For someday. The current setup is fine for now.

VincentSmedinga · 2023-12-08T09:08:29Z

The 'clear'-button currently uses native functionality, which only works in Chromium browsers (Chrome, Edge), not in Firefox for example. Is this functionality important enough to implement custom functionality for this?

I wouldn’t say so. Firefox users will be accustomed to not having a ‘clear’ button in a search field?

VincentSmedinga · 2023-12-08T11:22:32Z

proprietary/tokens/src/components/amsterdam/search-field.tokens.json

+        "outline-offset": { "value": "{amsterdam.focus.outline-offset}" },
+        "cancel-button": {
+          "background-image": {
+            "value": "url(\"data:image/svg+xml;utf8,<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 32 32'><path fill='%23004699' fill-rule='evenodd' d='M29.333 5.47 26.53 2.668 16 13.187 5.47 2.666 2.668 5.47 13.187 16 2.666 26.53l2.804 2.803L16 18.813l10.53 10.52 2.803-2.804L18.813 16z'/></svg>\")"


I can imagine using background icons more often. We could have an icons.tokens.json containing the value for path.d, or the entire svg, to allow reusing it later. We should extend the build step creating React components to also create these tokens.

Just an idea. For someday. The current setup is fine for now.

VincentSmedinga · 2023-12-08T11:25:40Z

packages/react/src/SearchField/SearchFieldInput.tsx

+          <VisuallyHidden>{label}</VisuallyHidden>
+        </label>
+        <input
+          {...restProps}


I think setting them to off initially is actually the best MVP approach – setting them on indeed has UX side-effects.

In general, we should be explicit in making HTML-attributes overridable or not – i.e. specifying them before or after {...restProps}. I made a note to discuss later.

This comment can be resolved.

VincentSmedinga · 2023-12-08T11:27:29Z

packages/react/src/SearchField/SearchField.tsx

+  ({ children, className, ...restProps }: SearchFieldProps, ref: ForwardedRef<HTMLFormElement>) => {
+    return (
+      <form role="search" {...restProps} ref={ref} className={clsx('amsterdam-search-field', className)}>
+        {children}


Yes, good reasons.

alimpens and others added 17 commits November 24, 2023 15:11

Scaffold component

687b388

Merge branch 'develop' of https://github.com/Amsterdam/design-system …

8faf831

…into feature/DES-507-mvp-search-field

Add Search Field component

4a784e8

Merge branch 'develop' of https://github.com/Amsterdam/design-system …

91e937d

…into feature/DES-507-mvp-search-field

Fix test

8d250bb

Change cancel button color, add input hover state, replace border wit…

85dd00e

…h box-shadow

Fix input focus outline

e554c21

Create subcomponents

829ee5b

Fix types

fc99b53

Isolate z-index to component

475f8ec

Prevent double click from zooming in on (some) touch devices

690c15f

Add docs

81feaa9

Add stories and docs

bede9cf

Split subcomponents, add test

471e95e

Add component to correct group in Storybook

718b00f

Temporarily remove controls

c2285e1

Merge branch 'develop' into feature/DES-507-mvp-search-field

577bc2f

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 6, 2023 11:36 Destroyed

alimpens requested review from VincentSmedinga and dlnr December 6, 2023 11:41

Add subcomponent controls

afdfbfb

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 6, 2023 18:49 Destroyed

VincentSmedinga reviewed Dec 7, 2023

View reviewed changes

Merge branch 'develop' into feature/DES-507-mvp-search-field

f4371e5

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 8, 2023 09:08 Destroyed

Add margin before cancel button

99686d6

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 8, 2023 10:13 Destroyed

Add test for controlled input component

50c9262

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 8, 2023 11:22 Destroyed

VincentSmedinga previously approved these changes Dec 8, 2023

View reviewed changes

Add docs on autocomplete and spellcheck

6088603

alimpens dismissed VincentSmedinga’s stale review via 6088603 December 8, 2023 12:25

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 8, 2023 12:25 Destroyed

Merge branch 'develop' into feature/DES-507-mvp-search-field

42940b9

github-actions bot temporarily deployed to demo-DES-507-mvp-search-field December 8, 2023 12:28 Destroyed

alimpens merged commit dd1a437 into develop Dec 8, 2023
4 checks passed

alimpens deleted the feature/DES-507-mvp-search-field branch December 8, 2023 12:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Search Field component #892

feat: Add Search Field component #892

alimpens commented Dec 6, 2023 •

edited

Loading

VincentSmedinga Dec 7, 2023

alimpens Dec 8, 2023

VincentSmedinga Dec 7, 2023

alimpens Dec 8, 2023

VincentSmedinga Dec 8, 2023

VincentSmedinga Dec 7, 2023

VincentSmedinga Dec 7, 2023

alimpens Dec 8, 2023

VincentSmedinga Dec 7, 2023

alimpens Dec 8, 2023

VincentSmedinga Dec 8, 2023

VincentSmedinga Dec 7, 2023

alimpens Dec 8, 2023

VincentSmedinga Dec 8, 2023

VincentSmedinga commented Dec 8, 2023

VincentSmedinga Dec 8, 2023

VincentSmedinga Dec 8, 2023

VincentSmedinga Dec 8, 2023

feat: Add Search Field component #892

feat: Add Search Field component #892

Conversation

alimpens commented Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentSmedinga commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alimpens commented Dec 6, 2023 •

edited

Loading