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

Add filters to trade history #544

Merged
merged 43 commits into from Nov 9, 2018

Conversation

Projects
None yet
2 participants
@kevva
Copy link
Member

kevva commented Oct 8, 2018

As the title says, this adds filters for date, symbol pairs and order type. Suggestions on how the design could be improved are welcome.

Also, the date input currently throws if you enter anything due to an internal function in react-day-picker which expects an event as the first argument onChange, but we give it (value, event). Not sure how we like to handle that.

screen shot 2018-10-08 at 02 36 02

kevva added some commits Oct 8, 2018

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 8, 2018

Looks very good to me. Just some minor nitpick:


If I write in the "Pair" input, the text is black and with too much left padding:

screen shot 2018-10-08 at 16 07 40


When hovering over the < and > in the calendar, it should not use a hand pointer.


The v here looks one pixel unaligned vertically:

screen shot 2018-10-08 at 16 12 50


If I select a date in "From" and "To" calendar, then open the "To" calendar and select a day inside the range, it doesn't update "To", but rather clears it.


Select 2018-10-18 in "From" and open the "To" calendar. Then try hovering over today's date and it disappears. It should have no hover effect like the rest of the disallowed dates.

@@ -44,8 +44,8 @@ module.exports = (env, options) => ({
options: require('./package.json').babel,
},
{
test: /\.scss$/,
exclude: /node_modules/,
test: /\.(s)?css$/,

This comment has been minimized.

Copy link
@sindresorhus

sindresorhus Oct 8, 2018

Member

I would prefer test: /\.(css|scss)$/, for readability.

@kevva

This comment has been minimized.

Copy link
Member Author

kevva commented Oct 8, 2018

When hovering over the < and > in the calendar, it should not use a hand pointer.

Fixed. Removed the pointer cursor from the calendar days too.

The v here looks one pixel unaligned vertically:

Indeed. I moved the arrow up one pixel.

If I select a date in "From" and "To" calendar, then open the "To" calendar and select a day inside the range, it doesn't update "To", but rather clears it.

I wanted it to clear upon clicking on the current date, not on the dates in range. Seems like clickUnselectsDay take all days in the range into account. Removed it for now.

Select 2018-10-18 in "From" and open the "To" calendar. Then try hovering over today's date and it disappears. It should have no hover effect like the rest of the disallowed dates.

Fixed.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 9, 2018

The date format should be 2018-10-02, not 2018-10-2

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 9, 2018

Can you upgrade to latest Yarn version and run $ yarn? I get some missing integrity hashes in the lockfile when I checkout this PR.

sindresorhus and others added some commits Oct 9, 2018

@kevva kevva changed the title [WIP] Add filters to trade history Add filters to trade history Oct 9, 2018

kevva added some commits Oct 9, 2018

Merge remote-tracking branch 'origin/master' into filter
* origin/master:
  Bump AVA and fix tests (#548)
  Add QMCoin (QMC) currency (#542)
  Add issue templates and instruction for adding a new currency (#540)
  Hide `Swap` view (#539)
@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 10, 2018

  • I think the square that appears when you hover over days in the calendar should have 2px in border-radius.
  • Validation for the manual input in the date fields.
@kevva

This comment has been minimized.

Copy link
Member Author

kevva commented Oct 10, 2018

Validation for the manual input in the date fields.

What should we show as placeholder? Should we skip From and To altogether and just have YYYY-MM-DD, alternatively an example date?

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 12, 2018

Thinking about this some more, I think it would be better to add default values there instead of placeholders. We could show To as today, and From as up to one year ago, or the date of the oldest entry if it's less than one year. That way it's clear to the user the format they need to use. We should also validate the input and only allow the pattern /\d{4}-\d{2}-\d{2}/.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 29, 2018

Currently, it's possible to to manually write 2019-10-24 in the From field. I don't think that should be allowed. How about we make it an error (red input) for a second and then revert it to to today's date?

Is it possible to prevent the From visual calendar from showing future dates?

kevva added some commits Oct 30, 2018

@kevva

This comment has been minimized.

Copy link
Member Author

kevva commented Oct 30, 2018

I forgot, we also need to validate that the input is an actual valid date, not just the pattern. 2018-10-50 is a valid pattern, but not date. We could either make pattern support a function too or add a type=date. The check could be something like moment('2018-10-50').isValid().

Hmm, I think accepting a function is the cleanest and most scalable way of doing it here. Will fix.

Currently, it's possible to to manually write 2019-10-24 in the From field. I don't think that should be allowed. How about we make it an error (red input) for a second and then revert it to to today's date?

Shouldn't it follow the same behaviour as the To input? I.e. that it gets a shake animation and then reverts the date.

Is it possible to prevent the From visual calendar from showing future dates?

Yes.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 31, 2018

Shouldn't it follow the same behaviour as the To input? I.e. that it gets a shake animation and then reverts the date.

Yeah, that's better. Just make sure it only reverts when the pattern is complete. You wouldn't want this behavior: User writes 2018-02, then stops to look up the day, then goes back and it's all gone.

Is it possible to prevent the From visual calendar from showing future dates?

Yes.

Let's do it then.

@kevva

This comment has been minimized.

Copy link
Member Author

kevva commented Oct 31, 2018

Should we do it for the To calendar too? No point in showing future dates if they're all disabled.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 31, 2018

Should we do it for the To calendar too?

👍

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 31, 2018

Yeah, that's better. Just make sure it only reverts when the pattern is complete. You wouldn't want this behavior: User writes 2018-02, then stops to look up the day, then goes back and it's all gone.

Sorry, I misunderstood when you validate. You validate on blur, not on debounce, so reverting on blur is fine even when it's incomplete, like 2018-02.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Oct 31, 2018

Tiny nitpick, but can make the endpoint here slightly less round? I looks slightly uneven now because of the large border radius.

screen shot 2018-10-31 at 17 28 12

@kevva kevva force-pushed the filter branch from 42f4b4b to 6222ec4 Nov 2, 2018

kevva added some commits Nov 1, 2018

Merge remote-tracking branch 'origin/master' into filter
* origin/master:
  Improve the check for whether Marketmaker is ready
  Add more logging (#565)
  Improve the price history fetching error handling
  Bump `ow` dependency
  Minor code style tweaks
  Improve error reporting when marketmaker times out
  Show 6 fractional numbers for currency less than 1
  Upgrade `recharts` dependency

@kevva kevva force-pushed the filter branch from 6222ec4 to fc3de31 Nov 4, 2018

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Nov 5, 2018

If I enter 2018-10-40 in From, and then blur the input, the time it takes to reset is too long. It feels almost like a mistake when it suddenly changes a second later. It should revert right after the shake animation.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Nov 5, 2018

If I enter 2018-10-99999999 in From, then blur the input, the input becomes red. I then select a valid date, and the input continues to be red.

@kevva

This comment has been minimized.

Copy link
Member Author

kevva commented Nov 5, 2018

If I enter 2018-10-40 in From, and then blur the input, the time it takes to reset is too long. It feels almost like a mistake when it suddenly changes a second later. It should revert right after the shake animation.

Yeah, agree. I'll lower the timeout slightly.

If I enter 2018-10-99999999 in From, then blur the input, the input becomes red. I then select a valid date, and the input continues to be red.

@sindresorhus, yes, problem is that onChange isn't triggered in Input when entering an invalid date in react-day-picker, thus why we have to update the internal state of it manually. Probably because of gpbl/react-day-picker#815. I think I have a workaround for it though.

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Nov 9, 2018

The validation/reset behavior feels good now 👌

@sindresorhus sindresorhus merged commit 6074f21 into master Nov 9, 2018

@sindresorhus sindresorhus deleted the filter branch Nov 9, 2018

@sindresorhus

This comment has been minimized.

Copy link
Member

sindresorhus commented Nov 9, 2018

Really nice work on this, @kevva. It turned out great! And it's going to be very useful for people doing a lot of trades.

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.