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.

Sign up for GitHub

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

Jump to bottom

Clean up focus background implementation #962

Merged
merged 5 commits into from
Aug 16, 2023
Merged

Clean up focus background implementation #962

merged 5 commits into from
Aug 16, 2023

Conversation

BSFishy
Copy link
Contributor

@BSFishy BSFishy commented Aug 14, 2023 •
edited
Loading

Description

Clean up some of the focus background implementation details

  • Make ouiFocusBackground mixin actually use $color argument
  • Make $ouiFocusTransparency an actual percentage instead of a float [0, 1]
  • Refactor any usage of $ouiFocusTransparency to respect it as a percentage instead of a float [0, 1]

Before

Screenshot (134)
Screenshot (135)
Screenshot (136)
Screenshot (137)
Screenshot (138)
Screenshot (139)
Screenshot (140)
Screenshot (141)

After

Screenshot (142)
Screenshot (143)
Screenshot (144)
Screenshot (145)
Screenshot (146)
Screenshot (147)
Screenshot (148)
Screenshot (149)

Related issues

Fixes #948

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

joshuarrrr
joshuarrrr reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@joshuarrrr joshuarrrr left a comment

Choose a reason for hiding this comment

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

looks good, but I'd like to get rid of "Transparency" in the var name.

Also need a changelog entry

src/global_styling/variables/_states.scss Outdated Show resolved Hide resolved
src/global_styling/variables/_states.scss Outdated
$ouiFocusTransparency: lightOrDarkTheme(.1, .3) !default;
$ouiFocusBackgroundColor: tintOrShade($ouiColorPrimary, ((1 - $ouiFocusTransparency) * 100%), ((1 - $ouiFocusTransparency) * 100%)) !default;
$ouiFocusTransparency: lightOrDarkTheme(90%, 70%) !default;
$ouiFocusBackgroundColor: tintOrShade($ouiColorPrimary, $ouiFocusTransparency, $ouiFocusTransparency) !default;
Copy link
Member

Choose a reason for hiding this comment

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

The simplification reveals that this definition itself is a bit suspect. If you search the OUI stylesheets for tintOrShade usage, you'll see that it's most common to specify more tint than shade (or, occasionally, the reverse). That makes sense from a color perception perspective.

There are only 2 other examples I found where the two percentages are the same:

oui/src/global_styling/variables/_buttons.scss

Line 16 in fe979c3

$ouiButtonColorDisabled: tintOrShade($ouiTextColor, 70%, 70%) !default;

oui/src/components/control_bar/_control_bar.scss

Line 177 in fe979c3

color: tintOrShade($colorValue, 30%, 30%);

So I don't think we should be afraid to further update this definition to be more in tune with other derived colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the point of tint or shade is more to specifically get the tint vs the shade functionality depending on dark or light colors. The actual color switching is done by the $ouiFocusTransparency variable, so we will only ever get tint 90% on light mode and shade 70% on dark mode. It's just separated into 2 separate variables

@BSFishy
Copy link
Contributor Author

BSFishy commented Aug 15, 2023

I changed the shade amount to 65%. It seems to be a little better but I cant check with next theme because some of the colors are detected as dark when they should be light and vice versa

@BSFishy
Update changelog
ecf56f3 
Signed-off-by: Matt Provost <[email protected]>
@KrooshalUX
Copy link
Contributor

Does this address #940 ? If so, LGTM!!!

@BSFishy
Copy link
Contributor Author

BSFishy commented Aug 16, 2023

Yep, also changes contrast :)

@BSFishy BSFishy merged commit 58380a5 into opensearch-project:main Aug 16, 2023
@BSFishy BSFishy deleted the focus/cleanup branch August 16, 2023 21:35
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 16, 2023
@github-actions
Clean up focus background implementation (#962)
106eef0 
* Clean up focus background implementation

Signed-off-by: Matt Provost <[email protected]>

* Rename variable to ouiFocusChangePercent

Signed-off-by: Matt Provost <[email protected]>

* Update changelog

Signed-off-by: Matt Provost <[email protected]>

---------

Signed-off-by: Matt Provost <[email protected]>
(cherry picked from commit 58380a5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Aug 16, 2023
Merged
BSFishy pushed a commit that referenced this pull request Aug 17, 2023
@opensearch-trigger-bot @github-actions
Clean up focus background implementation (#962) (#971)
48ddff6 
* Clean up focus background implementation

Signed-off-by: Matt Provost <[email protected]>

* Rename variable to ouiFocusChangePercent

Signed-off-by: Matt Provost <[email protected]>

* Update changelog

Signed-off-by: Matt Provost <[email protected]>

---------

Signed-off-by: Matt Provost <[email protected]>
(cherry picked from commit 58380a5)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

# Conflicts:
#	CHANGELOG.md

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joshuarrrr joshuarrrr joshuarrrr approved these changes

@AMoo-Miki AMoo-Miki AMoo-Miki approved these changes

@seanneumann seanneumann Awaiting requested review from seanneumann

@rednaksi91 rednaksi91 Awaiting requested review from rednaksi91

@KrooshalUX KrooshalUX Awaiting requested review from KrooshalUX

@bandinib-amzn bandinib-amzn Awaiting requested review from bandinib-amzn bandinib-amzn is a code owner

@ashwin-pc ashwin-pc Awaiting requested review from ashwin-pc ashwin-pc is a code owner

@andreymyssak andreymyssak Awaiting requested review from andreymyssak

@SergeyMyssak SergeyMyssak Awaiting requested review from SergeyMyssak

Assignees
No one assigned
Labels
backport 1.x distinguished-contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Increase contrast of keyboard focus highlight when applied to OuiLink
4 participants
@BSFishy @KrooshalUX @joshuarrrr @AMoo-Miki