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.

Sign up for GitHub

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

Jump to bottom

Responsive finance #619

Merged
merged 22 commits into from
Jan 28, 2019
Merged

Responsive finance #619

merged 22 commits into from
Jan 28, 2019

Conversation

AquiGorka
Copy link
Contributor

@AquiGorka AquiGorka commented Jan 7, 2019 •
edited
Loading

Pending:

  • Toggle animate Filters
  • New Transfer Token dropdown spans wider than available space

@AquiGorka AquiGorka added the wip label Jan 7, 2019
@AquiGorka AquiGorka requested review from bpierre and sohkai January 7, 2019 18:01
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.124% when pulling 85e5bb7 on feature/responsive-finance into 40b16f0 on feature/responsive-menu-panel.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.124% when pulling 85e5bb7 on feature/responsive-finance into 40b16f0 on feature/responsive-menu-panel.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.124% when pulling 85e5bb7 on feature/responsive-finance into 40b16f0 on feature/responsive-menu-panel.

@coveralls
Copy link

coveralls commented Jan 7, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 96.124% when pulling c7ecc0a on feature/responsive-finance into 8b07003 on master.

@bpierre bpierre mentioned this pull request Jan 7, 2019
Closed
40 tasks
@sohkai sohkai added this to the A1 Sprint: 3.1 milestone Jan 8, 2019
@AquiGorka AquiGorka force-pushed the feature/responsive-menu-panel branch 5 times, most recently from ff5fd8d to 7a9af30 Compare January 11, 2019 11:59
@sohkai sohkai modified the milestones: A1 Sprint: 3.1, A1 Sprint: 3.2 Jan 14, 2019
@AquiGorka AquiGorka force-pushed the feature/responsive-menu-panel branch 2 times, most recently from c39dc50 to efcec23 Compare January 14, 2019 11:20
@AquiGorka AquiGorka force-pushed the feature/responsive-finance branch from cefb097 to c7ecc0a Compare January 25, 2019 17:03
@AquiGorka
Copy link
Contributor Author

@bpierre

  • The IdentityBadge should be shortened earlier, so that the table doesn’t get cut
  • The labels should probably be wrapped so that they are not 1fr (50%) width
  • We should still have a minimum width (= horizontal scrollbar) when the viewport gets to a certain point (320px)
  • The toggle transition has a little jump at the end, and should use something like springs.smooth or springs.swift as a config
  • The buttons in the header should be closer to the edges, and I think it would be nice to have the entire area clickable (from top to bottom, and including the padding to the edge), especially for touch screens (agreed to come back to this and review all the spacing).

At certain sizes, we can see the button for the menu panel while it is still visible (maybe we could use GetWindowSize only for this?)

This is super hard when this happens the iframe's width is < 768 and there is no way to know if the menu panel exists and the window's width > 768. If it is alright with you I'll post this as an issue as it something that concerns all the iframe apps.

@bpierre
Copy link
Contributor

bpierre commented Jan 25, 2019

@AquiGorka Awesome!

If it is alright with you I'll post this as an issue as it something that concerns all the iframe apps.

👍

sohkai
sohkai reviewed Jan 28, 2019
View reviewed changes
apps/finance/app/src/App.js
height: 100%;
justify-content: space-between;
align-items: center;
justify-content: safe;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, does this need to be with an alignment keyword?

sohkai
sohkai reviewed Jan 28, 2019
View reviewed changes
apps/finance/app/src/components/Transfers.js
@@ -160,7 +197,7 @@ class Transfers extends React.Component {
key={transfer.transactionHash}
token={tokenDetails[toChecksumAddress(transfer.token)]}
transaction={transfer}
wideMode={width > 800}
wideMode={width > 768 + 219}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we should use a constant instead of 219 (is it the width of the side panel)?

Copy link
Contributor Author

@AquiGorka AquiGorka Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I did this on purpose so we would not miss it and it definitely needs to be improved.

is it the width of the side panel

Yes it is.

sohkai
sohkai reviewed Jan 28, 2019
View reviewed changes
apps/finance/app/src/components/Transfers.js
${breakpoint(
'medium',
`
margin: 10px 30px 20px 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is the right margin here necessary?

Actually, I was thinking of increasing the top margin to align it with the filters on larger screens. Thoughts @bpierre @AquiGorka @jounih?

Currently:

image

With more top margin (30px):

image

Copy link
Contributor Author

@AquiGorka AquiGorka Jan 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Can we push this to the backlog? And work it out with @jounih

Copy link

Choose a reason for hiding this comment

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

Yeah it should be aligned with the filters. Not sure you need that much margin though to separate the title & filters from the list, in the design is about this much:

screenshot 2019-01-28 at 23 12 53

Copy link
Contributor

Choose a reason for hiding this comment

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

Proposed fix: #767

@AquiGorka AquiGorka merged commit ca36c48 into master Jan 28, 2019
@AquiGorka AquiGorka deleted the feature/responsive-finance branch January 28, 2019 21:59
@luisivan luisivan mentioned this pull request Mar 4, 2019
Closed
46 tasks
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
@AquiGorka
Responsive finance (aragon#619)
c8fa0db 
* Finance: add mobile call to action

* Balances: add responsive styles for list

* BalanceRow: add responsive rules

* Transfers: add responsive rules for list

* TransferRow: add responsive behaviour

* Transfers: add filters to small screens

* Add react-spring dep

* Finance: add toggle filters button component

* Transfers: animate filters toggle

* TokenSelectorInstance: add responsive behaviour

* Finance: add responsive min width

* Move to styled v4 + @aragon/ui 0.30 (enable IdentityBadge popover)

* Align the app bar with the Token Manager app

* Spacing tweak

* Tweak the filters opening / close button + transition

- Use ButtonIcon.
- Use springs.smooth.
- Fix the shadow being cut at the end of the spring.

* Update new transfer icon

* Finance: fix non-function return to Transition

* ToggleContent: update to react spring latest api

* Transfers: remove filter title colons

* Balances: fix list style

* TransferRow: remove medium rules for small screen components
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jounih jounih jounih left review comments

@sohkai sohkai sohkai left review comments

@bpierre bpierre bpierre left review comments

@izqui izqui Awaiting requested review from izqui

@delfipolito delfipolito Awaiting requested review from delfipolito

Assignees
No one assigned
Labels
app: finance component: frontend
Projects
None yet
Milestone
A1 Sprint: 3.2
Development

Successfully merging this pull request may close these issues.
5 participants
@AquiGorka @coveralls @bpierre @sohkai @jounih