Skip to content

Storybook: Signature Request#10400

Merged
kumavis merged 3 commits intodevelopfrom
storybook-sig-req
Feb 8, 2021
Merged

Storybook: Signature Request#10400
kumavis merged 3 commits intodevelopfrom
storybook-sig-req

Conversation

@kumavis
Copy link
Copy Markdown
Member

@kumavis kumavis commented Feb 8, 2021

No description provided.

@kumavis kumavis requested a review from a team as a code owner February 8, 2021 15:16
@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

benchmark fails 😿

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 8, 2021

Builds ready [02b393b]
Page Load Metrics (611 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint46836394
domContentLoaded4137516097335
load4137516117335
domInteractive4127506097335

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

this is it ------------> https://242891-42009758-gh.circle-artifacts.com/0/storybook/index.html?path=/story/signature-request--first-look

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

network selector looks a little derpy, not sure why

@brad-decker
Copy link
Copy Markdown
Contributor

brad-decker commented Feb 8, 2021

For some reason the header is 85px tall when the header in the real app is only 58px. The side effect of the smaller height is that things just align the right way. When the height of the header is manually adjusted to 85px in the real app view, then the styling looks as it does in storybook.

adding display: flex, align-items: center to --network the same way it is added to --account is an appropriate fix for the alignment issue ... but still not quite sure why storybook renders the view with extra height on the header.

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021 •
edited
Loading

is the repeated Message label a bug in the confirmation or the story

screen shot

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021 •
edited
Loading

reproduced on https://metamask.github.io/test-dapp/

screen shot

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

issue is present on Sign Typed Data V3 + Sign Typed Data V4

@metamaskbot
Copy link
Copy Markdown
Collaborator

metamaskbot commented Feb 8, 2021

Builds ready [ee76412]
Page Load Metrics (806 ± 39 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint4812278178
domContentLoaded5379988048240
load5409998068239
domInteractive5369978038340

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

@brad-decker seems like a mistake, including the same label twice

metamask-extension/ui/app/components/app/signature-request/signature-request-message/signature-request-message.component.js

Lines 47 to 51 in b93046b

{this.context.t('signatureRequest1')}
</div>
<div className="signature-request-message--root">
<div className="signature-request-message__type-title">
{this.context.t('signatureRequest1')}

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

with updated styles for header https://242965-42009758-gh.circle-artifacts.com/0/storybook/index.html?path=/story/signature-request--first-look

@kumavis
Copy link
Copy Markdown
Member Author

kumavis commented Feb 8, 2021

this is good to go, above issue is out of scope

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Feb 8, 2021

The double-message-label problem is captured in #10124

@kumavis kumavis merged commit 85cf35b into develop Feb 8, 2021
@kumavis kumavis deleted the storybook-sig-req branch February 8, 2021 16:49
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@NiranjanaBinoy NiranjanaBinoy Awaiting requested review from NiranjanaBinoy

1 more reviewer

@brad-decker brad-decker brad-decker approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kumavis @metamaskbot @brad-decker @Gudahtt