[terra-search-field] Removed the terra-form-input and replaced it with a native input #3078

pranav300 · 2020-07-15T08:45:53Z

Summary

This PR removes the use of terra-form-input from the search-field and replaces it with native input.
All the styles necessary for the input are taken directly from terra-form-input.

This is done to support placeholder in search-field after PR #3075 has been merged, which removes placeholder from the form-input and form-textarea.

Part of #2992

Deployment Link

https://terra-core-search-field-asffbr.herokuapp.com/components/terra-search-field/search-field/search-field

Testing

Additional Details

Thank you for contributing to Terra.
@cerner/terra

packages/terra-search-field/src/SearchField.jsx

emilyrohrbough · 2020-07-17T20:10:42Z

@pranav300 I'm confused, why do we need to replace terra-form-input with native input to fix the placeholder issue? Don't we need to filter out and remove the placeholder prop from being passed down to the terra-form-input?

Also, is this PR ready for review? Looking at this issue, there are UX TO DOs for terra-seach-field: #2992 (comment)

pranav300 · 2020-07-17T20:51:53Z

@pranav300 I'm confused, why do we need to replace terra-form-input with native input to fix the placeholder issue? Don't we need to filter out and remove the placeholder prop from being passed down to the terra-form-input?

@emilyrohrbough
In terra-search-field, we do want to display placeholder and with the changes done in this PR #3075 it won't be possible with the terra-form-input.

This PR is not ready for review, I had raised it so that we can validate the approach.

emilyrohrbough · 2020-07-17T23:13:06Z

This PR is not ready for review, I had raised it so that we can validate the approach.

This PR is not ready for review, can we move this to draft mode? and/or add a no not merge label on this PR. There was nothing in the summary to indicate this wan't ready for review.

Since this isn't ready, who is suppose to be validating this? Could that person have looked at the branch diff?

packages/terra-search-field/CHANGELOG.md

StephenEsser · 2020-07-20T16:24:33Z

packages/terra-search-field/src/SearchField.module.scss

+      [dir=rtl] & {
+        box-shadow: var(--terra-search-field-input-focus-rtl-box-shadow);
+      }
+    }


We should reduce these styles/variables as much as possible and apply only the ones necessary for theming the search field appropriately. Are all terra-form-input variables required to theme the search field?

Are all terra-form-input variables required to theme the search field?

I had imported only the ones that were used by search-field when it imported form-input.
But still, I have gone through the CSS and removed some variables which I think were unnecessary here 468041b.

benbcai · 2020-07-20T20:55:15Z

packages/terra-search-field/src/SearchField.module.scss

+    width: 100%;
+
+    &::placeholder {
+      color: var(--terra-search-field-input-placeholder-color, rgba(25, 32, 35, 0.53));


Looks like this is the same placeholder color that was used in form-input. Wouldn't this continue to have the insufficient color contrast with the background that we originally had in form-input?

Updated here 7c7364d.

benbcai · 2020-07-22T19:57:43Z

packages/terra-search-field/src/SearchField.jsx

            aria-disabled={isDisabled}
            onKeyDown={this.handleKeyDown}
            onInput={this.handleInput}
-            refCallback={this.setInputRef}
+            ref={this.setInputRef}
            {...additionalInputAttributes}


Previously, form-input will automatically add aria-required if required was passed to inputAttributes. Since this is no longer leveraging form-input, it would lose this logic. Should this be added here?

I and @neilpfeiffer discussed this and decided that it is unneeded to make search-field a required field.

dkasper-was-taken · 2020-07-22T20:53:01Z

packages/terra-search-field/src/SearchField.jsx

            aria-disabled={isDisabled}
            onKeyDown={this.handleKeyDown}
            onInput={this.handleInput}
-            refCallback={this.setInputRef}
+            ref={this.setInputRef}
            {...additionalInputAttributes}


It maybe not be part of this scope, but in general we spread pass-through attributes before our implementation specific ones so the behaviors cannot be overridden. So I'd like to see this moved.

Thanks @dkasper-was-taken - logged this new issue here to move the spread to first in the list: #3086

neilpfeiffer · 2020-07-22T21:42:40Z

Tested: PR #3078
Part of Issue #2992 along with #3075

Reviewed:

Follows pattern considerations documented in Cerner Design Standards: Search: Global Search
Follows UX defined design implementation for Global Search
Follows defined theme designs: terra-default-theme, orion-fusion-theme, clinical-lowlight-theme

Fix with Followup Issues:

Related Updates (also will be updating usage of placeholders):
terra-core: issue #3083
terra-clinical: issue #680
terra-framework: issue #1193

Verified by @neilpfeiffer, ready to merge, merge before #3075,

Pass
UX Review

pranav300 added 3 commits July 15, 2020 13:37

updated to use native input

3990a42

update jest snapshots and changelog

fca32de

removed terra-form-input from dependencies

978b077

pranav300 added UX Review Required 📦 terra-search-field labels Jul 15, 2020

pranav300 requested review from neilpfeiffer, supreethmr, lokesh-0813, saket2403, ShettyAkarsh and avinashg1994 July 15, 2020 08:45

pranav300 requested review from dkasper-was-taken, mjhenkes, ryanthemanuel and tbiethman as code owners July 15, 2020 08:45

pranav300 self-assigned this Jul 15, 2020

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 15, 2020 08:46 Inactive

lokesh-0813 reviewed Jul 16, 2020

View reviewed changes

packages/terra-search-field/src/SearchField.jsx Outdated Show resolved Hide resolved

minor changes

e87bc5c

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 17, 2020 04:57 Inactive

pranav300 marked this pull request as draft July 18, 2020 05:52

StephenEsser reviewed Jul 20, 2020

View reviewed changes

packages/terra-search-field/CHANGELOG.md Outdated Show resolved Hide resolved

StephenEsser reviewed Jul 20, 2020

View reviewed changes

pranav300 mentioned this pull request Jul 20, 2020

[terra-form-input][terra-form-textarea] Remove prop placeholder #3075

Merged

benbcai reviewed Jul 20, 2020

View reviewed changes

pranav300 changed the title ~~[terra-search-field] Change terra-form-input to native input~~ [terra-search-field] Removed the terra-form-input and replaced it with a native input Jul 21, 2020

updated color for placeholder

7c7364d

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 21, 2020 08:01 Inactive

removed some unnecessary variables

468041b

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 21, 2020 09:32 Inactive

updated changelog

2ee19f2

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 21, 2020 20:20 Inactive

Merge branch 'main' into search-field-input

820d750

pranav300 marked this pull request as ready for review July 21, 2020 20:26

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 21, 2020 20:26 Inactive

pranav300 requested a review from emilyrohrbough July 21, 2020 20:27

Merge branch 'main' into search-field-input

d4564e3

mjhenkes temporarily deployed to terra-core-search-field-asffbr July 21, 2020 21:17 Inactive

benbcai reviewed Jul 22, 2020

View reviewed changes

benbcai approved these changes Jul 22, 2020

View reviewed changes

mjhenkes approved these changes Jul 22, 2020

View reviewed changes

dkasper-was-taken reviewed Jul 22, 2020

View reviewed changes

neilpfeiffer added the ⭐ UX Reviewed UX Reviewed and approved. label Jul 22, 2020

neilpfeiffer approved these changes Jul 22, 2020

View reviewed changes

ryanthemanuel merged commit 0b9f37a into main Jul 22, 2020

ryanthemanuel deleted the search-field-input branch July 22, 2020 22:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[terra-search-field] Removed the terra-form-input and replaced it with a native input #3078

[terra-search-field] Removed the terra-form-input and replaced it with a native input #3078

pranav300 commented Jul 15, 2020 •

edited by neilpfeiffer

Loading

emilyrohrbough commented Jul 17, 2020 •

edited

Loading

pranav300 commented Jul 17, 2020 •

edited

Loading

emilyrohrbough commented Jul 17, 2020

StephenEsser Jul 20, 2020

pranav300 Jul 21, 2020 •

edited

Loading

benbcai Jul 20, 2020

pranav300 Jul 21, 2020 •

edited

Loading

benbcai Jul 22, 2020

pranav300 Jul 22, 2020

dkasper-was-taken Jul 22, 2020

neilpfeiffer Jul 22, 2020

neilpfeiffer commented Jul 22, 2020 •

edited

Loading

[terra-search-field] Removed the terra-form-input and replaced it with a native input #3078

[terra-search-field] Removed the terra-form-input and replaced it with a native input #3078

Conversation

pranav300 commented Jul 15, 2020 • edited by neilpfeiffer Loading

Summary

Deployment Link

Testing

Additional Details

emilyrohrbough commented Jul 17, 2020 • edited Loading

pranav300 commented Jul 17, 2020 • edited Loading

emilyrohrbough commented Jul 17, 2020

StephenEsser Jul 20, 2020

Choose a reason for hiding this comment

pranav300 Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

benbcai Jul 20, 2020

Choose a reason for hiding this comment

pranav300 Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

benbcai Jul 22, 2020

Choose a reason for hiding this comment

pranav300 Jul 22, 2020

Choose a reason for hiding this comment

dkasper-was-taken Jul 22, 2020

Choose a reason for hiding this comment

neilpfeiffer Jul 22, 2020

Choose a reason for hiding this comment

neilpfeiffer commented Jul 22, 2020 • edited Loading

pranav300 commented Jul 15, 2020 •

edited by neilpfeiffer

Loading

emilyrohrbough commented Jul 17, 2020 •

edited

Loading

pranav300 commented Jul 17, 2020 •

edited

Loading

pranav300 Jul 21, 2020 •

edited

Loading

pranav300 Jul 21, 2020 •

edited

Loading

neilpfeiffer commented Jul 22, 2020 •

edited

Loading