Skip to content

[Fixed] #7921 Message text overflow removed redundant title#10485

Merged
Gudahtt merged 5 commits intoMetaMask:developfrom
BboyAkers:aakers/7921
Feb 26, 2021
Merged

[Fixed] #7921 Message text overflow removed redundant title#10485
Gudahtt merged 5 commits intoMetaMask:developfrom
BboyAkers:aakers/7921

Conversation

@BboyAkers
Copy link
Copy Markdown
Contributor

@BboyAkers BboyAkers commented Feb 22, 2021

Fixes: #7921

Explanation:
Message text overflow removed redundant title

Manual testing steps:

  • Connect to metaMask test dapp
  • Press sign button from SignTypedData_4

Result:

  • Text should warp and redundant message title should only show one message

@BboyAkers BboyAkers requested a review from a team as a code owner February 22, 2021 02:48
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Feb 22, 2021

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.

@BboyAkers
Copy link
Copy Markdown
Contributor Author

BboyAkers commented Feb 22, 2021 •
edited
Loading

Took me awhile to figure out the text wrapping but hopefully this suites the fix necessary and doesn't disturb the design. 🙂

Side note:
I was curious to know if both titles should be there. I saw they both use the same i18n context text. So my assumption was that we should delete one after digging further into everything.

Gudahtt
Gudahtt reviewed Feb 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a good start.

I'm still seeing a horizontal scrollbar in some cases though 🤔 Like with the signTypedData_v4 example on the test dapp.

Also, the indentation here is pretty severe. It won't take much nesting to make it entirely unreadable unfortunately. I don't know what to do about that though.

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Feb 22, 2021

See this PR for an earlier attempt at fixing this problem: #9165

Also, regarding the extra "Message" header, this draft PR addresses the same thing: #10406

@BboyAkers
Copy link
Copy Markdown
Contributor Author

BboyAkers commented Feb 23, 2021

Took awhile lol but there shouldn't be any overflow problems anymore!!! 🙂

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Feb 23, 2021

Thanks, this is definitely an improvement!

The indentation now seems slightly larger though, particularly for the first column. There's a lot of wasted space on the left.

The difference is slight, but when you consider that one level of indentation on develop was due to the rendundant Message header, the indentation is now way more than it should be. It would be ideal to bring all of this left to leave more room for the message.

Screenshots:

Before (develop):

sign-indent-old

After (this PR):

signed-indent

@BboyAkers
Copy link
Copy Markdown
Contributor Author

BboyAkers commented Feb 23, 2021

Gotcha, i'll get to that today and finish up!

@BboyAkers
Copy link
Copy Markdown
Contributor Author

BboyAkers commented Feb 26, 2021 •
edited
Loading

Sorry for the wait :P. Fixed the padding! 🙂 @Gudahtt

Gudahtt
Gudahtt approved these changes Feb 26, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit dedadee into MetaMask:develop Feb 26, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Feb 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@brad-decker brad-decker Awaiting requested review from brad-decker

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

signTypedData_v4 does not overflow:wrap fields.

2 participants

@BboyAkers @Gudahtt