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

[Enhancement]: Add support for custom filter #816

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nxjiten
Copy link
Collaborator

@nxjiten nxjiten commented Nov 28, 2019

@natabal

Add support for custom filters #814.

@nxjiten nxjiten requested a review from natabal November 28, 2019 14:09
@natabal
Copy link
Contributor

natabal commented Dec 4, 2019

@bmukheja Can you please test this?

@bharat-mukheja
Copy link
Contributor

Hi @nxjiten , so there is a bug in this -

  1. When the close button on the filter is clicked, it strips off all the custom variables in the url and sets the url to the default for the dashboard
  2. The rest of the urls on the dashboard have already been created with those custom variables included, so when we switch the dashboard, it reapplies the custom Time filter. Please see the below video for the demo.

Screen Recording 2019-12-05 at 1.08.39 PM.mov.zip

@nxjiten
Copy link
Collaborator Author

nxjiten commented Dec 6, 2019

@bmukheja
I have fixed the issue mentioned in point 1 and working on point 2 issue which might take some time to fix.

@bharat-mukheja
Copy link
Contributor

@nxjiten - That's okay, you can take your time. I haven't tested the new change yet, will test it today and comment.

@nxjiten
Copy link
Collaborator Author

nxjiten commented Dec 12, 2019

@bmukheja, the second issue is also fixed. Please have a look and let me know.

@bharat-mukheja
Copy link
Contributor

@nxjiten - This error is happening now

index.js:1437 Warning: Failed prop type: Invalid prop deleteIconStyle of type string supplied to Chip, expected object.
in Chip (at Chips.js:18)
in Chips (at FiltersToolBar/index.js:417)
in li (at FiltersToolBar/index.js:416)
in ul (at FiltersToolBar/index.js:430)
in div (at FiltersToolBar/index.js:429)
in FiltersToolBarView (created by Context.Consumer)
in Connect(FiltersToolBarView) (at Dashboard/index.js:204)
in div (at Dashboard/index.js:201)
in DashboardView (created by Context.Consumer)
in Connect(DashboardView) (created by Route)
in Route (at App.js:18)
in div (at AppContainer.js:62)
in div (at AppContainer.js:59)
in div (at AppContainer.js:79)
in MuiThemeProvider (at AppContainer.js:78)
in AppContainerView (created by Context.Consumer)
in Connect(AppContainerView) (at App.js:16)
in MuiThemeProvider (at App.js:15)
in App (at src/index.js:15)
in Router (created by ConnectedRouter)
in ConnectedRouter (created by Context.Consumer)
in ConnectedRouterWithContext (created by Context.Consumer)
in Connect(ConnectedRouterWithContext) (at src/index.js:14)
in Provider (at src/index.js:13)

@nxjiten
Copy link
Collaborator Author

nxjiten commented Dec 23, 2019

@bmukheja
I have fixed the warning in the Chip. Please have a look.

@nxjiten
Copy link
Collaborator Author

nxjiten commented Dec 23, 2019

@natabal @bmukheja

Hi Bharat,
As discussed with Natalia, please assign all the issues reported in visualization-framework.

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.

3 participants