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

Finance filters refresh #767

Merged
merged 9 commits into from Apr 17, 2019

Conversation

Projects
None yet
4 participants
@bpierre
Copy link
Member

bpierre commented Apr 5, 2019

Builds on #765 (don’t merge before).

Rationale

  • The download button has been designed as a DropDown to support several export formats, but only one format (CSV) is supported at the moment. The issue is that the DropDown style has been kept while the component now behaves like a button. The new version uses the same style we have everywhere else for icon buttons.
  • The row is now too crowded to be displayed properly at all sizes. The new version is moving the filters row below the title in large views, as it was already the case in smaller views.
  • The labels have been updated: “Date range” becomes “Period”, and “Transfer type” becomes “Type” (we are in the “Transfers” filters so it is already implied).
  • I think a proper redesign would still be welcomed, as it makes little sense to have the download button with the filters, and way the filters layout is not optimal.

Before

filters-before-320
filters-before-600
filters-before-1000
filters-before-1450

After

filters-after-320
filters-after-600
filters-after-1000
filters-after-1450

bpierre added some commits Apr 4, 2019

Improve the filters layout
- Move TransfersFilters to its own component.
- Make the layout work at all sizes.
- Update the download button so it looks like the other icon buttons.
- Update some labels: “Date range” => “Period”, “Transfer type” =
  “Type” (since it is already below the “Transfers” title).

@bpierre bpierre requested review from sohkai and dizzypaty Apr 5, 2019

@bpierre bpierre changed the title Finance filters Finance filters refresh Apr 5, 2019

@dizzypaty
Copy link
Member

dizzypaty left a comment

These changes are a great improvement to the general layout of the screen and the 'Export' action is now consistent with the style and behavior of the icon button component 🚀🤘

@luisivan luisivan referenced this pull request Apr 5, 2019

Closed

Responsive #4

39 of 46 tasks complete

@bpierre bpierre referenced this pull request Apr 5, 2019

Merged

Responsive finance #619

2 of 2 tasks complete

@bpierre bpierre requested a review from 2color Apr 5, 2019

@sohkai

sohkai approved these changes Apr 5, 2019

Copy link
Member

sohkai left a comment

🥇

@dizzypaty I do like having the "button" look to the download button; right now I get a bit confused if I can actually click on it or not.

On the mobile view as well, what do you think about centering the icon in the cell?

@@ -128,7 +123,7 @@ class Transfers extends React.PureComponent {
const { selectedDateRange } = this.state
const start = format(selectedDateRange.start, 'yyyy-MM-dd')
const end = format(selectedDateRange.end, 'yyyy-MM-dd')
return `Finance_(${start}_to_${end}).csv`
return `transfers_${start}_to_${end}.csv`

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Could we use the Finance app's proxy address here, like 0x1a0adf8909(Finance)_transfers_...

transferTypeFilter={selectedTransferType}
onTransferTypeChange={this.handleTransferTypeChange}
compactMode={compactMode}
opened={filtersOpened}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Could we move compactMode and opened to the top of the props list?

compactMode={compactMode}
opened={filtersOpened}
symbols={symbols}
transferTypes={TRANSFER_TYPES_STRING}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Could we rename symbols to tokenSymbols, and move it together with the token filter and onchange handler?

Similarly, could we move transferTypes to be with the transfer type filter and onchange handler?

key={transfer.transactionHash}
token={tokenDetails[toChecksumAddress(transfer.token)]}
transaction={transfer}
smallViewMode={compactMode}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Maybe we can also rename smallViewMode to compactMode?

Show resolved Hide resolved apps/finance/app/src/components/TransfersFilters.js
margin: 0;
justify-content: space-between;
margin-left: 0;
margin-right: 0;

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Seems like we don't need these two margin declarations

{filteredTransfers.length === 0 ? (
<NoTransfers>
<p>
No transfers found.{' '}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 5, 2019

Member

Could we also add the date period here? I think it's a bit confusing that we automatically apply the dates so a lot of users will just see "No transfers found." if they haven't made a transaction in the current month.

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 7, 2019

Author Member

What about making it clearer that filters are applied in general?

No transfers found with the current filters.
[Clear current filters?]

/cc @dizzypaty

This comment has been minimized.

Copy link
@dizzypaty

dizzypaty Apr 8, 2019

Member

Yes, I agree with making it general – how about we say:

"No transfers match your filter selection. Clear filters"

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Member

I still think it'd be really useful to show the date range being applied, because at first glance it'll appear immediately broken if there's no transactions and the user knows their finance app had transactions previously.

This is especially true on mobile, where the filters are hidden by default.

Another problem is that we'd need a definition for what "clear filters" mean for the date range. Should we just set it to Nov of last year (since it's when it launched)? Should we be smarter and set it based on the app's initialization block time (actions are only possible on the app after it's been initialized)?

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 9, 2019

Author Member

Oh right, I was assuming the date range was not applied until the user selects one!

@sohkai I think your solution would be an improvement in the current state of things, but it would still be quite confusing on mobile with the current design in some cases: if there is let’s say one item, and the others are hidden, nothing would indicate the current date range as it is only displayed in the empty state.

About the “clear filters”: what about the case where a specific date range has been selected, but also another filter? Do we reset the date range as well? Or do we consider it not being a filter?

I think it could be easier for the time being to only apply the date range once the user selects it (having the field empty by default), like the other filters, at least until this part gets redesigned.

Thoughts @dizzypaty @sohkai?

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 9, 2019

Member

I think it could be easier for the time being to only apply the date range once the user selects it (having the field empty by default), like the other filters, at least until this part gets redesigned.

I think this would be preferable, but does the component allow us to do this? It might look weird being completely blank as its default state.

This comment has been minimized.

Copy link
@dizzypaty

dizzypaty Apr 9, 2019

Member

@sohkai, @bpierre I also assumed none of filters were applied until users selection.

Having a look at this with more detail now:

  • Date range should be deselected by default (until user makes a date selection)
  • We should indicate today’s date with a slightly different visual style than the date selection – will do a quick mock-up!
  • We should include a ‘Clear all’ button, that removes the current filter selection

This comment has been minimized.

Copy link
@dizzypaty

dizzypaty Apr 11, 2019

Member

@sohkai , @bpierre Based on the previous comments, these are some changes to the date range filter:

Default state (no filters applied, calendar displays today's date)

WebApp-1366px - Finance main view

Date range selected

WebApp-1366px - Finance main view-1

Filters applied

WebApp-1366px - Finance main view-2

Note: after trying a couple of things yesterday, I think we can leave the 'Clear filters' button out of these changes for the release and then rethink the layout properly.

Figma file

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 5, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 0ecf00e on fix-finance-filters into e5078ff on master.

3 similar comments
@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 5, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 0ecf00e on fix-finance-filters into e5078ff on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 5, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 0ecf00e on fix-finance-filters into e5078ff on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 5, 2019

Coverage Status

Coverage remained the same at 96.734% when pulling 0ecf00e on fix-finance-filters into e5078ff on master.

@bpierre

This comment has been minimized.

Copy link
Member Author

bpierre commented Apr 7, 2019

@dizzypaty I do like having the "button" look to the download button; right now I get a bit confused if I can actually click on it or not.

If we decide to go with a “solid” button style, I think we should also align the filters button with it, and have a consistent button style for “icon buttons on the blue background” (assuming the confusion comes from having these buttons on the blue background?).

On the mobile view as well, what do you think about centering the icon in the cell?

Everything else (the filters on the left, and the filter button above it) is either left or right aligned. Having this specific button slightly misaligned with the filters button above wouldn’t look good IMO.

But what might look a bit weird is that we are aligning the header icons on the illustrations directly, while the icon buttons are aligned on the button around it, including its padding:

For these cases, I think we could shift these buttons so they look aligned, the same way we do it already with text buttons. The IconButton has 5px padding on each side, so we could shift it 5px on either side it is aligned:

And after the icons internal spacing will be more consistent (see aragon/aragon-ui#346), we could also move these buttons 2px or 3px more, so that they would look as aligned as possible with the gutter.

But if we decide to use another style for these (the filters toggle and the export button), then we could also move the label inside of it (to have an icon + label button), and maybe only have the icon in smaller views.

@sohkai

This comment has been minimized.

Copy link
Member

sohkai commented Apr 8, 2019

Mmm the left / right alignment makes sense 👍.

assuming the confusion comes from having these buttons on the blue background

Yes, I think so. The other filters "pop out" with a box shadow and look like they're meant to be interacted with.

@dizzypaty

This comment has been minimized.

Copy link
Member

dizzypaty commented Apr 8, 2019

@dizzypaty I do like having the "button" look to the download button; right now I get a bit confused if I can actually click on it or not.

I agree @sohkai and I think we need to explore a different visual style for these icon buttons in general, even more so if they're displayed on the blue background. We need to define a rationale for our layer and depth logic (mainBG, buttonsBG, popovers/modals, etc.) and keep it consistent so it's clearer when UI elements are interactive. Until we do this, my inclination would be to keep the way we present these icon buttons consistent, and we have 3 of them in this screen.

bpierre added some commits Apr 17, 2019

@dizzypaty
Copy link
Member

dizzypaty left a comment

👍🏻

@sohkai

sohkai approved these changes Apr 17, 2019

Copy link
Member

sohkai left a comment

✔️ ☑️

@dizzypaty Can you make a new issue for the default state of the date picker?

@bpierre

This comment has been minimized.

Copy link
Member Author

bpierre commented Apr 17, 2019

:shipit: :shipit: :shipit: :shipit: :shipit: :shipit:

@bpierre bpierre merged commit 08c9f75 into master Apr 17, 2019

1 of 2 checks passed

License Compliance FOSSA is analyzing this commit
Details
license/cla Contributor License Agreement is signed.
Details

@bpierre bpierre deleted the fix-finance-filters branch Apr 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.