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

chore: Update bottom border style for application header #29334

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gambinish
Copy link
Contributor

@gambinish gambinish commented Dec 18, 2024

Description

Changes bottom shadow of app header to a border-bottom as issue specifies.

Open in GitHub Codespaces

Related issues

Fixes: #27313

Manual testing steps

Screenshots/Recordings

Screenshot 2024-12-18 at 3 27 19 PM Screenshot 2024-12-18 at 3 26 34 PM

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@gambinish gambinish marked this pull request as ready for review December 18, 2024 23:28
@gambinish gambinish requested a review from a team as a code owner December 18, 2024 23:28
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

vinnyhoward
vinnyhoward previously approved these changes Dec 19, 2024
@gambinish gambinish requested a review from a team as a code owner December 19, 2024 20:50
@gambinish gambinish changed the title chore: Update style for header, update snapshots chore: Update bottom border style for application header Dec 19, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [9a6cbac]
Page Load Metrics (1777 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint14942076176815976
domContentLoaded14832013174514971
load15252024177714972
domInteractive22178433517
backgroundConnect1078362612
firstReactRender1691422713
getState522942
initialActions01000
loadScripts10041600128615675
setupStore668222110
uiStartup176825842070215103
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@@ -100,10 +100,10 @@ describe('Test Snap Notification', function () {

// try to click on the account menu icon (via xpath)
await driver.clickElement(
'[data-testid="account-options-menu-button"]',
'[data-testid="notifications-tag-counter__unread-dot"]',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was complaining because the dot was obscuring the button

.multichain-app-header-shadow {
box-shadow: var(--shadow-size-md) var(--color-shadow-default);
.multichain-app-header-border {
border-bottom: 1px solid var(--color-border-muted);
Copy link
Member

Choose a reason for hiding this comment

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

I'm really confused by this change, we use box-shadow for other headers too, e.g. the Snaps confirmation headers. If anything this will make that feel more disconnected. I thought we were trying to use the box-shadow look everywhere?

Perhaps @eriknson can chime in

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the issue for reference, I'm open to adding this elsewhere if we need to: #27313

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Design quality]: Update header styling for Portfolio View
5 participants