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 tx history list grid layout #7272

Closed
wants to merge 2 commits into from
Closed

Conversation

danjm
Copy link
Contributor

@danjm danjm commented Oct 10, 2019

Fixes #7036

Screenshot from 2019-10-10 14-26-49

Screenshot from 2019-10-10 14-24-40

@metamaskbot
Copy link
Collaborator

Builds ready [b742a20]

@danjm danjm force-pushed the fix-tx-history-padding branch 2 times, most recently from 60b04c6 to d471578 Compare October 22, 2019 12:20
@danjm
Copy link
Contributor Author

danjm commented Oct 22, 2019

@Gudahtt I made some changes in response to your comments

@metamaskbot
Copy link
Collaborator

Builds ready [d471578]

@metamaskbot
Copy link
Collaborator

Builds ready [d471578]

@danjm danjm force-pushed the fix-tx-history-padding branch from d471578 to 3ff9a5e Compare October 23, 2019 14:14
"nonce nonce nonce"
"identicon action primary-amount"
"identicon status secondary-amount";
grid-template-columns: 45px 145px 3fr;
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that you're still omitting the fourth column here. Is there any reason for that? I'm not even sure what behavior to expect in this case, e.g. whether it defaults to 1fr or something else. Certainly if you don't specify something for the fourth column, using 3fr seems rather pointless ("3 parts" of the remaining space, when those are the only parts)

Setting the second column as 145px seems reasonable though - I can see why you did that. I do wonder whether that's long enough to fit any of the messages that might appear there though 🤔. I'm not sure where to find a full list of those, particularly the translated versions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh right, the fourth column...

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, it defaults to 1fr Just pushed a commit which makes that explicit

Fitting long messages is an existing problem on that screen, with or without that change. Which should be addressed through design, but probably not in this issue?

Copy link
Member

Choose a reason for hiding this comment

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

Sure, of course. I was more concerned about the possibility that this change made it worse (i.e. truncated a message that previous was not truncated). I don't think we need to worry about solving the general problem in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately there is still a regression here:

Current develop branch:
develop

This branch:
clipped

The only status long enough to exceed the 145px is "Contract Deployment"; it looks like it needs at least 153px to fit without being truncated.

Though more importantly, this has brought my attention to a more substantial change; that it was three columns before, and is now four. The status was placed below the transaction category, not alongside it.

I would suggest restoring the third column, and leaving the status below the transaction category. Then there would be ample room to accommodate not only "Contract Deployment", but any translated category names that might be substantially longer.

@metamaskbot
Copy link
Collaborator

Builds ready [3ff9a5e]

@metamaskbot
Copy link
Collaborator

Builds ready [2703c73]

@danjm
Copy link
Contributor Author

danjm commented Oct 28, 2019

Closing in favour of #7319

@danjm danjm closed this Oct 28, 2019
@whymarrh whymarrh deleted the fix-tx-history-padding branch October 29, 2019 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Padding in TX History
3 participants