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

Notifications UI: add all filter to table #989

Merged
merged 5 commits into from Sep 10, 2019

Conversation

@2color
Copy link
Contributor

commented Sep 9, 2019

No description provided.

@2color 2color requested a review from sohkai Sep 9, 2019

@auto-assign auto-assign bot requested review from AquiGorka and bpierre Sep 9, 2019

event: events[selectedEvent],
appName: subscriptionApps[selectedApp],
organization: organizations[selectedOrganization],
event: selectedEvent > 0 ? events[selectedEvent] : null,

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 9, 2019

Member

In addition to doing this, we should also default the selectedEvent, selectedApp and selectedOrganization state to be -1 (when we select 'All'), so that it deactivates the filter.

This comment has been minimized.

Copy link
@2color

2color Sep 9, 2019

Author Contributor

@sohkai
Not sure I understood but do you mean selecting -1, i.e. the dropdown label when All is selected?

If so, I've pushed a commit for that.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 9, 2019

Member

Yep, that's what he meant. We' ve been doing it this way: when the user selects All, we actually set the index to -1 (as if none was selected). When none, is selected then apply no filter and the filter does not show the blue outline for a (non -1) selected option.
Hope this helps clarify.

@sohkai
sohkai approved these changes Sep 9, 2019
Copy link
Member

left a comment

Ready to be merged 😄!

@@ -67,20 +71,20 @@ const SubscriptionsTable = React.memo(function SubscriptionsTable({
}, [])
const onOrganizationChange = useCallback(idx => {

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 10, 2019

Member

All of these handlers should reset the pagination no? Is there pagination actually? We might be missing it.

This comment has been minimized.

Copy link
@sohkai

sohkai Sep 10, 2019

Member

Pagination is included by DataView by default, and if you don't control it from outside, it manages its own state. It does not yet reset the pagination when the items change, but discussing if it should.

This comment has been minimized.

Copy link
@AquiGorka

AquiGorka Sep 10, 2019

Member

discussing if it should?

I do think it should, it would be very weird (and I saw it that's why I manually reset pages on other filters) for the user to apply a filter and not see results because of being on the wrong page.

@sohkai sohkai merged commit 536f070 into master Sep 10, 2019

2 of 6 checks passed

install install
Details
License Compliance FOSSA is analyzing this commit
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
now Now is deploying your app
Details
WIP Ready for review
Details

@sohkai sohkai deleted the notification-all-filter branch Sep 10, 2019

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