-
Notifications
You must be signed in to change notification settings - Fork 245
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
UI: Optimize tx-table for mobile screen #1804
UI: Optimize tx-table for mobile screen #1804
Conversation
✅ Deploy Preview for specter-desktop-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@relativisticelectron could you perhaps rebase this PR on this PR #1813 (once it is merged) - I've done some work there on the tx table which should also make it a bit more mobile friendly. |
Sure. |
…ilescreen # Conflicts: # src/cryptoadvance/specter/templates/includes/tx-table.html
Done. Now this PR is much smaller. |
@moneymanolis : Could you review this PR? It is now really small. |
Will do! |
@@ -200,6 +200,7 @@ | |||
width: 100%; | |||
margin-bottom: 10px; | |||
border: 1.5px solid var(--cmap-border); | |||
overflow-x: scroll; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think overflow-x: auto would be better choice, considering it handles both overflowing and non overflowing cases. Just to avoid a scroll bar in case of no-overflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for responding so late: I would have chosen a different approach. I would have added the class optional
to the time column, similarly to what we have done with the txid column (or to another column that seems not so necessary for limited space on a phone). We ca finetune that later in a different PR if you like the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@relativisticelectron Would look like this, let me know what you think, I can quickly make a PR out of it:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would have worked too :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -353,6 +353,7 @@ | |||
locked: this.tx.locked, | |||
} }); | |||
this.dispatchEvent(event); | |||
e.stopPropagation(); // this prevent the onclick of outer divs to be triggered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@relativisticelectron could you explain this? Which onclick listeneres picked that up? Where did that bubble to? I thought it would be contained within the web component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire row becomes clickable
if (window.innerWidth < 690) { |
The propagation to the outer div has to be stopped.
Before:
After:
While not 100% perfect. It is now usable also on a very narrow screen.
TODOs: