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

Fix table isn't displayed with date filter #3842

Merged
merged 5 commits into from
May 29, 2019
Merged

Fix table isn't displayed with date filter #3842

merged 5 commits into from
May 29, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented May 28, 2019

What type of PR is this? (check all applicable)

  • Bug Fix

Description

Fix for #3836

Related Tickets & Documents

Fixes #3836

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Filter
filter-example

Multifilter
multifilter-example

@gabrieldutra
Copy link
Member Author

@arikfr there are two commits of mine:

  • First one - fix the issues I found using String to compare objects
  • Second one - implements my idea of using the keys

Comparing keys approach with the string one, the good part is that it uses Moment values for comparison, thus it's more reliable. Also if another type of value appear in the future the string approach may not cover it. The con I see is that I had to introduce the { key, label } syntax from Antd select's labelInValue because I couldn't define the value otherwise, this made this fix a bit more complex.

So, lmk what you prefer.

@kravets-levko
Copy link
Collaborator

@gabrieldutra There is some issue with searching values in filter's dropdown - when I open dropdown and try to type something in the input - it sometimes shows matching filter values, and sometimes not (but I know that there are values matching my input).

@kravets-levko
Copy link
Collaborator

@gabrieldutra BTW - Select.Option has two attributes: key and value. key can be only string or number, but value can be anything. If value is present, Select will use it instead of key. I think you can use it for "raw" values (and put formatted as Option's content) - it should simplify your code and fix search.

@gabrieldutra
Copy link
Member Author

Thanks for point this out @kravets-levko.

The issue with the search is that it works on the value || key attr by default, it will be necessary to modify the filterOption prop with a search function 😪

Select.Option has two attributes: key and value. key can be only string or number, but value can be anything

I'd tried that with value, it does "allow" you to send raw values, but it has a propType warning:

propTypes.

I wouldn't trust Ant internal API to handle those values without errors internally (e.g: on search)

@arikfr
Copy link
Member

arikfr commented May 29, 2019

LGTM. @kravets-levko wdyt?

Copy link
Collaborator

@kravets-levko kravets-levko left a comment

Choose a reason for hiding this comment

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

Thanks! 🎉

@gabrieldutra
Copy link
Member Author

It seems that Percy still has some weird diffing, at least it became rare. Well, that one looks normal both local and on Preview 🙂

@arikfr
Copy link
Member

arikfr commented May 29, 2019

Good to go? :)

@gabrieldutra gabrieldutra merged commit c2e31f0 into master May 29, 2019
@denisov-vlad
Copy link
Member

Thanks.

But I think that DATE column should be without hours and minutes. It looks a little bit noisy in multifilter.

@arikfr
Copy link
Member

arikfr commented May 30, 2019

@denisov-vlad while I agree, it felt like a reasonable compromise for now. Would you mind opening an issue for this, so we don't forget to address it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Table isn't displayed with Date filter
4 participants