Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-search-field] Update search field focus indicator to standard dashed line #3834

Merged
merged 17 commits into from
Jul 26, 2023

Conversation

trandrew1023
Copy link
Contributor

@trandrew1023 trandrew1023 commented Jun 30, 2023

Summary

What was changed:
Changed the search field's focus indicators to standard Terra dashed line for Terra themes. For Orion Fusion theme, there is still ongoing discussion about the styling so the only change for Orion Fusion is adding the box shadow to the input field to match the buttons to increase the visibility of the focus indicator.

Why it was changed:
Focus indicator consistency with other components

Testing

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Terra Default:
image
image
image

Terra Lowlight:
image
image
image

Orion Fusion:
image
image
image

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-8109


Thank you for contributing to Terra.
@cerner/terra

@trandrew1023 trandrew1023 self-assigned this Jul 7, 2023
@trandrew1023 trandrew1023 marked this pull request as ready for review July 7, 2023 18:12
@trandrew1023 trandrew1023 requested a review from a team as a code owner July 7, 2023 18:12
@trandrew1023 trandrew1023 changed the title Update search field focus indicator to standard dashed line [terra-search-field] Update search field focus indicator to standard dashed line Jul 7, 2023
Comment on lines 51 to 52
outline: var(--terra-search-field-focus-keyboard-outline, 2px dashed #000);
outline-offset: var(--terra-search-field-focus-keyboard-outline-offset, 1px);
Copy link
Contributor

@cm9361 cm9361 Jul 7, 2023

Choose a reason for hiding this comment

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

This pattern is showing up a lot in our code with the accessibility updates. Should this be converted to a mixin that can be reused or some other reusable concept? It is used 3 times in this file alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline with Charles and team: will need to discuss as team the default focus indicators for each theme and go forward strategy to implement in a way that all components will receive a consistent styling

@@ -47,7 +47,9 @@
}

&:focus {
border-color: var(--terra-search-field-button-focus-border-color, #26a2e5);
box-shadow: var(--terra-search-field-clear-focus-box-shadow);
outline: var(--terra-search-field-focus-keyboard-outline, 2px dashed #000);
Copy link
Contributor

@cm9361 cm9361 Jul 7, 2023

Choose a reason for hiding this comment

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

In this scenario, can't we create one descendant focus pseudo selector to cover all the use cases under the search field? I don't think we should have to add a selector for each type of element. I think this could make it so that you only have to maintain one style block for the focus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cm9361 I was able to add the styles under the *.focus here 4e9be89

Not sure if there's another approach you would apply here outside of the discussion had about having a library wide focus indicator style

@@ -9,7 +9,7 @@ Terra.describeViewports('Search Field', ['medium'], () => {
});

it('should enter a search term', () => {
$('input').setValue('Lore');
$('input').setValue('Lore', { mismatchTolerance: 0.1 });
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, what is the reason for the mismatchTolerance being set??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those tests were randomly failing due to some of the edges of the focus dotted lines being darker/lighter than the reference

image

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense. Thanks!

Can you add a comment indicating why this was added for documentation purposes? Something like:
Setting the mismatchTolerance to 0.1 to account for subtle variances in the dotted line.

(Or whatever wording is more accurate)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Added comments for the new mismatchTolerances here 0ef9818

@@ -2,6 +2,9 @@

## Unreleased

* Changed
* Updated focus indicators to standard dashed line.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Updated focus indicators to standard dashed line.
* Updated focus indicators to be a standard dashed line.

Copy link
Contributor Author

@trandrew1023 trandrew1023 Jul 13, 2023

Choose a reason for hiding this comment

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

Comment on lines 59 to 60
--terra-search-field-input-focus-keyboard-outline: none;
--terra-search-input-field-focus-keyboard-outline-offset: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--terra-search-field-input-focus-keyboard-outline: none;
--terra-search-input-field-focus-keyboard-outline-offset: none;
--terra-search-field-input-focus-keyboard-outline: none;
--terra-search-input-field-focus-keyboard-outline-offset: none;

not able to find these variables on other themes !! are these variables required ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, these are no longer needed and have been removed here d61bf2c

Copy link
Contributor

@eawww eawww left a comment

Choose a reason for hiding this comment

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

Beautiful! 😙👌

@github-actions github-actions bot temporarily deployed to preview-pr-3834 July 26, 2023 02:40 Destroyed
@sdadn sdadn merged commit 1d2e43d into main Jul 26, 2023
21 checks passed
@sdadn sdadn deleted the search-field-focus-updates branch July 26, 2023 14:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants