Finance: move to Viewport #671
Conversation
bpierre
commented
Feb 14, 2019
bpierre
commented
- Remove uses of BreakPoint.
- Remove GetWindowSize.
- Moved the app layout structure into AppLayout.
- Moved the main action icon into an icon component.
- Remove uses of BreakPoint. - Remove GetWindowSize. - Moved the app layout structure into AppLayout. - Moved the main action icon into an icon component.
| <TableHeader /> | ||
| </TableRow> | ||
| </BreakPoint> | ||
| <Viewport> |
There was a problem hiding this comment.
Do we need this extra Viewport when we already have an instance above?
| @@ -199,12 +206,12 @@ class Transfers extends React.Component { | |||
| key={transfer.transactionHash} | |||
| token={tokenDetails[toChecksumAddress(transfer.token)]} | |||
| transaction={transfer} | |||
| wideMode={width > 768 + 219} | |||
| wideMode={width > breakpoints.medium + 219} | |||
There was a problem hiding this comment.
Could we set a constant for 219?
There was a problem hiding this comment.
I ended up removing this entirely, since we don’t want IdentityBadge to be in wide mode anymore.
| <Viewport> | ||
| {({ below }) => ( | ||
| <AppView | ||
| padding={below('medium') ? smallViewPadding : 30} |
There was a problem hiding this comment.
Maybe we could add a largeViewPadding that defaults to 30?
| <SidePanel | ||
| <div css="min-width: 320px"> | ||
| <Main> | ||
| <AppLayout |
| </Grid> | ||
| </StyledTableCell> | ||
| </TableRow> | ||
| ) : ( |
There was a problem hiding this comment.
I would say in this case it might make sense to have explicit if/else blocks, since the tree is so large and it's hard to spot the :
| </ContextMenuItem> | ||
| </ContextMenu> | ||
| )} | ||
| {showCopyTransferMessage && ( |
There was a problem hiding this comment.
Noticed you removed the commented out context menu (don't remember why we commented it out in the first place; maybe iframe permissions?), but should we just remove all of the code related to it?
I find copying the transaction hash to be more useful, and worst case opening the etherscan URL gives you that anyway.
There was a problem hiding this comment.
Ah yes. I’m not sure why we decided to remove it, maybe for iframe permissions yes. I’m going to remove the code, and we’ll probably have a better solution soon within the transaction popover.
| @@ -205,7 +202,7 @@ class Transfers extends React.PureComponent { | |||
| key={transfer.transactionHash} | |||
| token={tokenDetails[toChecksumAddress(transfer.token)]} | |||
| transaction={transfer} | |||
| wideMode={width > WIDE_MODE_FROM} | |||
| smallViewMode={below('medium')} | |||
There was a problem hiding this comment.
cc @AquiGorka on this, just to be sure this is safe 😄
There was a problem hiding this comment.
Oh yeah these two props seem similar but are unrelated:
wideModeis an old one, that was used to make the badge shortened / not shortened (it was the first use ofGetWindowSizethat becameViewport😁), and we don’t need it anymore since the badge should always be shortened now.smallViewModeis for the compact mode that @AquiGorka introduced: I made it as an external prop to not have unnecessary instances ofViewport.
| afterTitle: PropTypes.node, | ||
| onMenuOpen: PropTypes.func.isRequired, | ||
| smallViewPadding: PropTypes.number, | ||
| largeViewPadding: PropTypes.number, |
There was a problem hiding this comment.
Same comment as on the other PR, should this be mediumViewPadding?
- Upgrade @aragon/ui to 0.31.0. - Move to Viewport (remove uses of BreakPoint). - Remove GetWindowSize. - Use Main. - Moved the app layout structure into a dedicated component, AppLayout. - Moved the main action icon into an icon component. - The IdentityBadge is now always shortened.
