Skip to content

fix: Apply stacked flashbar feature shadows to correct element#728

Merged
jperals merged 1 commit intomainfrom
jkuelz-flashbar-fix-after
Feb 9, 2023
Merged

fix: Apply stacked flashbar feature shadows to correct element#728
jperals merged 1 commit intomainfrom
jkuelz-flashbar-fix-after

Conversation

@jkuelz
Copy link
Member

@jkuelz jkuelz commented Feb 8, 2023

Description

The recent shadow updates in #702, unified how we apply shadows in many components, including simplifying the shadow approach in Flashbars. Previously, Flashbars applied shadows on different elements depending on the theme (pseudo element in refresh, directly on the Flash item in classic). However the latest update simplified both themes to use a single approach (pseudo element) (see code diff).

This unintentionally brought a tiny existing bug to light in the unreleased stacked Flashbar feature. More specifically, stacked Flashbars should have a different shadow compared to a normal un-stacked Flashbar shadow. This was the case in classic but not refresh - because shadows previously were applied to separate elements, the shadow was overriding the original shadows in classic (intended), but it was adding them to the default Flashbar shadow in visual refresh resulting in a double shadow (not intended).

This bug was barely noticeable to the human eye due to the large spread of shadows in visual refresh, which is why it did not get caught until now. The refactoring in #702, brought this the same bug that was happening in visual refresh to appear in classic as well, resulting in a double shadow on all stacked Flashbars (in classic and refresh - previously just refresh), and is more noticeable in classic as seen in the screenshots below (albeit still very subtle).

This change also addresses a bug in dark mode visual refresh shadow that was causing it to overlap underlying Flashbars, making them almost invisible.

Screenshots below, each highlighting:

before #702 | after #702 | after #702 with this fix

Screen Shot 2023-02-08 at 10 05 13 AM copy 2

Screen Shot 2023-02-08 at 10 03 14 AM copy

Screen Shot 2023-02-08 at 10 53 13 AM

Screen Shot 2023-02-08 at 10 03 47 AM copy

Related links, issue #, if available: n/a

How has this been tested?

Locally, part of the reason this regression happened in the first place was because there were no tests in place to catch it. These are coming in a separate PR as part of the stacked Flashbar feature release.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jkuelz jkuelz requested a review from a team as a code owner February 8, 2023 19:01
@jkuelz jkuelz requested review from connorlanigan and removed request for a team February 8, 2023 19:01
@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Base: 93.27% // Head: 93.27% // No change to project coverage 👍

Coverage data is based on head (bc8f68c) compared to base (76e0135).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #728   +/-   ##
=======================================
  Coverage   93.27%   93.27%           
=======================================
  Files         584      584           
  Lines       17074    17074           
  Branches     4703     4703           
=======================================
  Hits        15926    15926           
  Misses       1068     1068           
  Partials       80       80           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@jperals jperals merged commit b21ebba into main Feb 9, 2023
@jperals jperals deleted the jkuelz-flashbar-fix-after branch February 9, 2023 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants