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

fix: Snap Section component overflow #29882

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

Conversation

hmalik88
Copy link
Contributor

@hmalik88 hmalik88 commented Jan 23, 2025

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions:

  1. What is the reason for the change? Text inside a section component does not wrap and instead overflows.
  2. What is the improvement/solution? Make sure flex-wrap: wrap is applied to the section (box) component and that each text component has horizontal space with flex: 1.

Related issues

Fixes: #29876

Manual testing steps

  1. Pull down this branch and build the extension.
  2. Trigger custom ui content with the example code in the issue above.
  3. Observe the fix.

Screenshots/Recordings

Before

See issue

After

Screenshot 2025-01-23 at 12 31 40 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.

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.

@metamaskbot metamaskbot added the team-snaps-platform Snaps Platform team label Jan 23, 2025
@hmalik88 hmalik88 marked this pull request as ready for review January 23, 2025 17:38
@hmalik88 hmalik88 requested a review from a team as a code owner January 23, 2025 17:38
@metamaskbot
Copy link
Collaborator

Builds ready [bfbce8e]
Page Load Metrics (1697 ± 111 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39223101639364175
domContentLoaded141722901678221106
load142123211697231111
domInteractive2399442211
backgroundConnect75121157
firstReactRender1598382512
getState475182110
initialActions00000
loadScripts10071570121715675
setupStore584152110
uiStartup164427121959304146
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 40 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [941fb9e]
Page Load Metrics (1733 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint30420591664333160
domContentLoaded15232016170411254
load15462068173313062
domInteractive2310138199
backgroundConnect1087292110
firstReactRender17101402814
getState578222210
initialActions01000
loadScripts10471527124610651
setupStore686192311
uiStartup181626812055221106
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 40 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@hjetpoluru hjetpoluru self-requested a review January 29, 2025 16:02
Copy link
Contributor

@hjetpoluru hjetpoluru left a comment

Choose a reason for hiding this comment

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

Tested the changes locally and working as expected and code LGTM!

@@ -49,5 +50,6 @@ export const box: UIComponentFactory<BoxElement> = ({
alignItems: element.props.center && AlignItems.center,
className: 'snap-ui-renderer__panel',
color: TextColor.textDefault,
flexWrap: FlexWrap.Wrap, // ensures components can wrap horizontally
Copy link
Member

Choose a reason for hiding this comment

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

Both this change and the flex change will have impact beyond the overflow we are trying to fix here.

Have you tested existing Snap UI's to make sure this doesn't cause regressions in behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tested it with the box being vertical and behavior was unchanged. If you're talking about regressions across all existing snap interfaces (examples and production snaps), no -- that's a lot to check. This and other work has made me think if we should have some service that checks for regressions programmatically, realistically any UI change can break an existing snap interface.

@metamaskbot
Copy link
Collaborator

Builds ready [1daff52]
Page Load Metrics (1735 ± 72 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22719511604474228
domContentLoaded14651931170614369
load14691970173515072
domInteractive267837157
backgroundConnect897292512
firstReactRender1580372412
getState45312136
initialActions00000
loadScripts10421433122312259
setupStore760212010
uiStartup16982316200717886
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 40 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Text overflows outside the box when there is huge content in a section
4 participants