Add min-width support to Box DS component; Fix Blockaid Security Alert overflow issue; Fix missing Box width-max breakpoint styles #21317

digiwand · 2023-10-11T08:51:52Z

Description

Fix Blockaid Security Alert Overflow Issue
Fix missing ".mm-box--width-max" breakpoint styles. Discovered while adding min-width styles
Add min-width support to Box DS component to support fix
Add 0 value to $fractions

Manual testing steps

One way to test this:

yarn storybook
go to Blockaid Security Alert page
update "Features" value with the example or another long text.
example:

"The address 0x07Be9763a718C0539017E2Ab6fC42853b4aEeb6B is malicious"

Screenshots/Recordings

Before

After

Related issues

Fixes #21316

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.

github-actions · 2023-10-11T08:52:04Z

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.

garrettbear · 2023-10-11T16:41:41Z

LGTM, looks like need to update another snapshot

georgewrmarshall

This looks really great! Because the Box component is central to all of our component library components we are super sensitive to changes. ~~Would it be possible to add some unit tests to ensure coverage is 100% for the minWidth prop?~~ Then we can merge. It would also be great to add some documentation so other engineers can use this technique when they come across the issue but that could be another PR

Edit: I was expecting to see this prop not covered in jest coverage but it looks like it is so maybe coverage was the wrong term here. Nevertheless I think it would be great to add a unit test to test his prop

Co-authored-by: George Marshall <[email protected]> :) adding after syncing & realizing we created nearly the same code here

digiwand · 2023-10-11T20:30:12Z

@georgewrmarshall good call! Thank you. Funny we had nearly the same code when we synced 🚀 also since you're here and I think you built a lot of this, it was really interesting following how the code for this system works!

re: documenting the technique
Happy to add it somewhere if there's a good location for it. I'm not sure where would be a good place though. I will also say that it is a common solution to managing flex elements. Not intuitive though fairly easy to find searching for flex overflow issues

georgewrmarshall · 2023-10-12T09:44:33Z

ui/pages/confirm-add-suggested-nft/__snapshots__/confirm-add-suggested-nft.test.js.snap

@@ -1,7 +1,7 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

 exports[`ConfirmAddSuggestedNFT Component should match snapshot 1`] = `
-{
+Object {


Unsure of why Object has been added here?

hmm not sure but it caused the test to fail. I ran yarn test:unit:jest:watch --updateSnapshot* earlier which I believe added this

This was interesting because it was added by --updateSnapshot. When I ran the test locally, the test wasn't failing with Object.

georgewrmarshall

LGTM! Really nice work

not sure why it's failling in my PR rather than before this

not sure why yarn lint:fix added this

digiwand · 2023-10-12T12:16:16Z

ui/pages/confirm-add-suggested-nft/confirm-add-suggested-nft.js

@@ -240,6 +240,7 @@ const ConfirmAddSuggestedNFT = () => {
                  return (
                    <Box
                      className="confirm-add-suggested-nft__nft-single"
+                      key={`confirm-add-suggested-nft__nft-single-${id}`}


Fixed this during the quest to fix CI/CD tests.

digiwand · 2023-10-12T17:59:09Z

quite the stubborn tests 😅

running yarn test:unit:jest:watch --updateSnapshots stalls at ~341 out of 568 test suites. When they fail in CircleCI I'm only seeing the latest test failure. When I update an individual test, it has been prepending Objector Array on the test wrapper. Need to remove and then "Save without formatting". Something is also strange with my VSCode. I disabled ESLint, Prettier, and the related settings, and still, when I save, it will remove the white lines from the .snap files.

guess I'm doing these one at a time until I can unlock a better way

codecov · 2023-10-12T18:19:41Z

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (d6d324b) 68.61% compared to head (f9ddc7c) 68.61%.
Report is 4 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop   #21317   +/-   ##
========================================
  Coverage    68.61%   68.61%           
========================================
  Files         1020     1020           
  Lines        40929    40929           
  Branches     10920    10920           
========================================
  Hits         28080    28080           
  Misses       12849    12849

Files	Coverage Δ
...ert/blockaid-banner-alert/blockaid-banner-alert.js	`95.83% <100.00%> (ø)`
...ents/component-library/banner-base/banner-base.tsx	`100.00% <ø> (ø)`
ui/components/component-library/box/box.tsx	`100.00% <ø> (ø)`
ui/helpers/constants/design-system.ts	`100.00% <ø> (ø)`
...irm-add-suggested-nft/confirm-add-suggested-nft.js	`86.30% <ø> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

metamaskbot · 2023-10-12T18:31:08Z

Builds ready [f9ddc7c]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5, 6
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 12, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1255 ± 404 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	93	229	115	29	14
		domContentLoaded	77	194	103	25	12
		load	88	2200	1255	840	404
		domInteractive	77	194	103	25	12

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 113 Bytes (0.00%)
common: 0 Bytes (0.00%)

digiwand · 2023-10-12T19:19:11Z

the tests are passing 🤘🏼 Thanks for the reviews! May we get 2 more reviews/approvals again plz? ty in advance
@DDDDDanica @garrettbear @georgewrmarshall @jpuri

digiwand added 3 commits October 10, 2023 20:38

feat:DS: add Box minWidth support

5acdd5b

fix:DS missing mm-box--width-max breakpoint styles

99e916e

fix: See details overflow in banner alert

7e08f44

digiwand added team-design-system All issues relating to design system in Extension team-confirmations-secure-ux-PR PRs from the confirmations team needs-ux-ds-review labels Oct 11, 2023

digiwand requested a review from a team as a code owner October 11, 2023 08:51

digiwand added 2 commits October 11, 2023 10:53

fix:lint box.scss needed new lines

40c31e5

fix:unit:test BannerTip snapshot

f007c1e

jpuri previously approved these changes Oct 11, 2023

View reviewed changes

DDDDDanica previously approved these changes Oct 11, 2023

View reviewed changes

fix: BannerAlert related snapshots

d18484f

digiwand dismissed stale reviews from DDDDDanica and jpuri via d18484f October 11, 2023 19:46

georgewrmarshall requested changes Oct 11, 2023

View reviewed changes

test: add Box min-width tests

d57e463

Co-authored-by: George Marshall <[email protected]> :) adding after syncing & realizing we created nearly the same code here

fix:test: remove-snap-accounts snaphot

26005f1

georgewrmarshall reviewed Oct 12, 2023

View reviewed changes

georgewrmarshall previously approved these changes Oct 12, 2023

View reviewed changes

fix: ConfirmAddSuggestedNFT missing key

cc30ffe

not sure why it's failling in my PR rather than before this

digiwand dismissed georgewrmarshall’s stale review via cc30ffe October 12, 2023 12:08

fix: undo Object in ConfirmAddSuggestedNFT snap

1fcf697

not sure why yarn lint:fix added this

digiwand commented Oct 12, 2023

View reviewed changes

digiwand added 4 commits October 12, 2023 14:34

fix: ConfirmAddSuggestedNft update snap

4e1922c

fix:test snap rm Object

b24164f

Merge branch 'develop' into fix-blockaid-banner-alert-overflowing-text

a8e6d73

fix: readd whitespace ConfirmAddSuggestedNFT snap

8aeab9d

digiwand added 2 commits October 12, 2023 19:38

fix:test: ConfirmSendEther snapshot

f70b0fc

fix:test: EthSignModal snapshot

f9ddc7c

digiwand requested review from jpuri, georgewrmarshall and DDDDDanica October 12, 2023 19:11

georgewrmarshall approved these changes Oct 13, 2023

View reviewed changes

segun approved these changes Oct 13, 2023

View reviewed changes

jpuri approved these changes Oct 13, 2023

View reviewed changes

brad-decker merged commit a3b78f0 into develop Oct 13, 2023
9 checks passed

brad-decker deleted the fix-blockaid-banner-alert-overflowing-text branch October 13, 2023 14:36

github-actions bot locked and limited conversation to collaborators Oct 13, 2023

metamaskbot added the release-11.5.0 Issue or pull request that will be included in release 11.5.0 label Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add min-width support to Box DS component; Fix Blockaid Security Alert overflow issue; Fix missing Box width-max breakpoint styles #21317

Add min-width support to Box DS component; Fix Blockaid Security Alert overflow issue; Fix missing Box width-max breakpoint styles #21317

digiwand commented Oct 11, 2023 •

edited

Loading

github-actions bot commented Oct 11, 2023

garrettbear commented Oct 11, 2023

georgewrmarshall left a comment •

edited

Loading

digiwand commented Oct 11, 2023

georgewrmarshall Oct 12, 2023

digiwand Oct 12, 2023 •

edited

Loading

digiwand Oct 12, 2023 •

edited

Loading

georgewrmarshall left a comment

digiwand Oct 12, 2023

digiwand commented Oct 12, 2023

codecov bot commented Oct 12, 2023 •

edited

Loading

metamaskbot commented Oct 12, 2023

digiwand commented Oct 12, 2023

Add min-width support to Box DS component; Fix Blockaid Security Alert overflow issue; Fix missing Box width-max breakpoint styles #21317

Add min-width support to Box DS component; Fix Blockaid Security Alert overflow issue; Fix missing Box width-max breakpoint styles #21317

Conversation

digiwand commented Oct 11, 2023 • edited Loading

Description

Manual testing steps

Screenshots/Recordings

Before

After

Related issues

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Oct 11, 2023

garrettbear commented Oct 11, 2023

georgewrmarshall left a comment • edited Loading

Choose a reason for hiding this comment

digiwand commented Oct 11, 2023

georgewrmarshall Oct 12, 2023

Choose a reason for hiding this comment

digiwand Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

digiwand Oct 12, 2023 • edited Loading

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

digiwand Oct 12, 2023

Choose a reason for hiding this comment

digiwand commented Oct 12, 2023

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

metamaskbot commented Oct 12, 2023

digiwand commented Oct 12, 2023

digiwand commented Oct 11, 2023 •

edited

Loading

georgewrmarshall left a comment •

edited

Loading

digiwand Oct 12, 2023 •

edited

Loading

digiwand Oct 12, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading