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!: Use new Button features for SearchField #1695

Merged
merged 9 commits into from
Nov 6, 2024

Conversation

VincentSmedinga
Copy link
Contributor

Describe the pull request

Comments in Search Field suggested reusing Icon Button. Now that we’re replacing that with an icon-only button, I wondered if that could be a drop-in replacement. And… it is! I found no visual or other regressions.

What

Use Icon Button for Search Field Button.

Why

Reuse existing components.

How

  • Replaced the custom HTML button with Icon Button and remove associated styles and tokens.
  • Decided to keep SearchField.Button as a better API than letting consumers use IconButton explicitly in SearchField.
  • Decided to disallow the icon, iconPosition and variant props on Search Field Button, although having a visible ‘Zoeken’ label doesn’t seem bad either.

Checklist

Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:

  • Add or update unit tests
  • Add or update documentation
  • Add or update stories
  • Add or update exports in index.* files
  • Start the PR title with a Conventional Commit prefix, as explained here.

Additional notes

  • Should we refactor SearchField.Input to SearchInput and make this thing a pattern?

@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button October 23, 2024 11:22 Destroyed
Copy link
Contributor

@dlnr dlnr left a comment

Choose a reason for hiding this comment

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

Seems like a logical change, which can also be done in Figma. But i don't get the extra layer of SearchField.Button.

RubenSibon
RubenSibon previously approved these changes Nov 1, 2024
Copy link
Contributor

@RubenSibon RubenSibon left a comment

Choose a reason for hiding this comment

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

LGTM, but do address @dlnr's comment about using the Button component directly.

alimpens
alimpens previously approved these changes Nov 4, 2024
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 15:50 Destroyed
@VincentSmedinga VincentSmedinga dismissed stale reviews from alimpens and RubenSibon via 747ef9e November 5, 2024 15:56
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 15:56 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 16:23 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 19:33 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 19:39 Destroyed
@github-actions github-actions bot temporarily deployed to demo-DES-1005-refactor-search-field-button November 5, 2024 19:58 Destroyed
@alimpens alimpens merged commit 7b275d0 into develop Nov 6, 2024
6 checks passed
@alimpens alimpens deleted the feature/DES-1005-refactor-search-field-button branch November 6, 2024 10:41
@github-actions github-actions bot mentioned this pull request Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants