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.

Sign up for GitHub

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

Jump to bottom

Invalid filters #375

Merged
merged 15 commits into from
Jan 14, 2019
Merged

Invalid filters #375

merged 15 commits into from
Jan 14, 2019

Conversation

Ro4052
Copy link
Contributor

@Ro4052 Ro4052 commented Jan 10, 2019

#349 UI filters should give user feedback when they are valid

  • Exposed new table method for determining if a filter is valid
  • Viewer validates filters before passing to table
  • Invalid filters operands are underlined in red with a red exclamation mark next to them
  • Filtering by an empty string is now invalid (this seems more appropriate for the current viewer UI)
  • Added a screenshot test for the new invalid filter highlighting
  • Added test for filtering by empty string
  • Added tests for newly exposed is_valid_filter method

invalid_filter_exclamation

Ro4052 added 15 commits January 4, 2019 10:38
@timkpaine timkpaine added enhancement Feature requests or improvements JS labels Jan 10, 2019
@texodus
Copy link
Member

texodus commented Jan 14, 2019

Thanks for the PR! This is a great addition - User experience is awesome, and the tests in particular are greatly appreciated!

I think as a future enhancement we may want to reconsider this API, in favor of general validation across APIs which take configuration objects as parameters - see also my comments on API differences between viewer and library. I think this will help keep the API surface small, predictable, as well as portable and consistent between host languages.

As this is currently undocumented, IMO it is fine as is for now.

@texodus texodus merged commit df251b9 into finos:master Jan 14, 2019
@Ro4052 Ro4052 deleted the invalid-filters branch January 15, 2019 09:18
@Ro4052 Ro4052 mentioned this pull request Jan 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
enhancement Feature requests or improvements JS
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ro4052 @texodus @jburton-scottlogic @timkpaine