Correct message when all datasources filtered out #112

wojta · 2018-08-17T13:47:07Z

https://issues.jboss.org/browse/AEROGEAR-7791

Motivation

Better UX.

What

Searching the data sources - when searching for an item and it's not found show a No item found on search type message.

Why

As of writing this ticket it shows the blank slate message which could be confusing for the user.

Verification Steps

Display page with datasources, when there is none, it should show No Data Sources defined page
Add some datasources
It should show the datasources in the list
Try to filter it by name so the filter no longer matches any of them
It should show page No item found while filtering the data sources and button Clear the filter
When pressing Clear the filter filter is cleared and all datasources are displayed

Checklist:

Code has been tested locally by PR requester
Changes have been successfully verified by another team member

Progress

Displaying different messages
Possibility to clear filter quickly

Additional Notes

PS.: Add images and/or .gifs to illustrate what was changed if this pull request modify the appearance/output of something presented to the users.

cfoskin

Functionality works fine. Code looks good also, but lets wait for @josemigallas to review that first before merging

josemigallas · 2018-08-23T14:19:31Z

@wojta 2 things

It would be good to have Chris' feedback before we change the current UI, I for instance would not add a clear filter button but would keep the create one.
Implementation wise, EmptyList have changed so much it is now even less flexible. Instead of adding new prop whenFiltered why not extract the customizable values ({ title, info, action, actionName }) so that this component could be easy to reuse? action and actionName could be optional. And then even DefaultEmptyList could be merged with all this and make a single component.

josemigallas · 2018-08-23T14:31:35Z

ui/components/common/CommonToolbar.js

@@ -50,6 +52,8 @@ class CommonToolbar extends Component {
                        type="text"
                        placeholder="Filter by Name"
                        className={toolbarFilter}
+                        value={get(this.props.filter, "name", "")}


I think you can avoid installing the new package for this, perhaps destructuring:

const { onFilter, filter: { name } } = this.props;

I understand that. I'm not very experienced with JavaScript, but I think that this destructuring is particularly very confusing thing to do in this case, especially for someone coming from other languages.

josemigallas · 2018-08-23T14:34:19Z

ui/components/resolvers/ResolverDetail.js

@@ -19,6 +19,7 @@ import { DeleteResolverDialog } from "./DeleteResolverDialog";
 import { DefaultEmptyView } from "../common/DefaultEmptyView";

 import UpsertResolver from "../../graphql/UpsertResolver.graphql";
+import GetDataSources from "../../graphql/GetDataSources.graphql";


I think the branch needs rebasing

josemigallas · 2018-08-23T14:51:11Z

ui/components/common/CommonToolbar.js

@@ -50,6 +52,8 @@ class CommonToolbar extends Component {
                        type="text"
                        placeholder="Filter by Name"
                        className={toolbarFilter}
+                        value={get(this.props.filter, "name", "")}
+                        style={{ height: "26px" }}


This should be declared in .toolbarFilter css class, but I think it's not needed anyway.

josemigallas · 2018-08-23T14:53:43Z

ui/components/data-sources/DataSourcesContainer.js

@@ -53,6 +53,10 @@ class DataSourcesContainer extends Component {
        this.setState({ showDeleteModal: true, selectedDataSource: dataSource });
    }

+    clearFilter() {
+        this.setState({ filter: {} });


Why is filter an object?

It was like that previously, someone probably thought that there might be filters by more criteria.

It's true, my bad!

wtrocki · 2018-08-24T10:00:05Z

@wojta Please rebase and update this. We could merge that after

josemigallas · 2018-08-24T14:15:33Z

ui/components/common/CommonToolbar.js

@@ -47,6 +49,8 @@ class CommonToolbar extends Component {
                type="text"
                placeholder="Filter by Name"
                className={toolbarFilter}
+                value={get(this.props.filter, "name", "")}


Why you need lodash.get?

Isn't there some other way than destructuting? I really don't like it.

I'm sure you'll get used to it after a few tries! Unfortunately, I don't think installing a whole new package just to avoid an unliked built-in feature of ES6 is justified :/

ok, then :/

josemigallas · 2018-08-24T14:15:50Z

ui/components/common/CommonToolbar.js

@@ -47,6 +49,8 @@ class CommonToolbar extends Component {
                type="text"
                placeholder="Filter by Name"
                className={toolbarFilter}
+                value={get(this.props.filter, "name", "")}
+                style={{ height: "26px" }}


This should removed or at least included in the css class.

josemigallas · 2018-08-24T14:23:16Z

ui/components/data-sources/DataSourcesContainer.js

-        let name = nameToFilter;
-        if (name === "") {
-            name = undefined;
+        const { name, ...newFilter } = this.state.filter;


I don't think this change is needed

Actually it's needed for clearing the filter.

wtrocki · 2018-08-24T15:18:31Z

Got that running and checked the code.
@josemigallas Could you take look on this as well.

…erogear/data-sync-ui into AEROGEAR-7791-filtered-out-datasources

Changes to PR 112

wtrocki · 2018-08-24T20:56:07Z

Awesome work Josemi

wojta requested review from josemigallas and pb82 as code owners August 17, 2018 13:47

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch 2 times, most recently from e168664 to 174b2be Compare August 17, 2018 14:30

wojta changed the title ~~[WIP] Correct message when all datasources filtered out~~ Correct message when all datasources filtered out Aug 17, 2018

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch 2 times, most recently from a3490b3 to 6d8dafd Compare August 17, 2018 14:39

cfoskin reviewed Aug 20, 2018

View reviewed changes

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch from 6d8dafd to 03058a1 Compare August 23, 2018 12:57

josemigallas suggested changes Aug 23, 2018

View reviewed changes

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch from 03058a1 to 99e51a4 Compare August 24, 2018 13:27

josemigallas reviewed Aug 24, 2018

View reviewed changes

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch 2 times, most recently from e3540e5 to 3094daf Compare August 24, 2018 14:58

Correct message when all datasources filtered out

1f56e03

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch from 3094daf to 1f56e03 Compare August 24, 2018 14:59

Correct message when all datasources filtered out

fb47a0a

wojta force-pushed the AEROGEAR-7791-filtered-out-datasources branch from 1f56e03 to fb47a0a Compare August 24, 2018 15:31

josemigallas and others added 6 commits August 24, 2018 19:33

Merge branch 'AEROGEAR-7791-filtered-out-datasources' of github.com:a…

26aa85e

…erogear/data-sync-ui into AEROGEAR-7791-filtered-out-datasources

Correct message when all datasources filtered out

0872f93

fixes

35faaf7

changes icon

c783cee

Merge pull request #116 from josemigallas/PR-112

f85d8dd

Changes to PR 112

adds some tests

e37f6c7

josemigallas merged commit 4907038 into master Aug 24, 2018

josemigallas deleted the AEROGEAR-7791-filtered-out-datasources branch August 24, 2018 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct message when all datasources filtered out #112

Correct message when all datasources filtered out #112

wojta commented Aug 17, 2018 •

edited

cfoskin left a comment

josemigallas commented Aug 23, 2018

josemigallas Aug 23, 2018

wojta Aug 23, 2018 •

edited

josemigallas Aug 23, 2018

josemigallas Aug 23, 2018

josemigallas Aug 23, 2018

wojta Aug 23, 2018

josemigallas Aug 23, 2018

wtrocki commented Aug 24, 2018

josemigallas Aug 24, 2018

wojta Aug 24, 2018 •

edited

josemigallas Aug 24, 2018 •

edited

wojta Aug 24, 2018

josemigallas Aug 24, 2018

wojta Aug 24, 2018

josemigallas Aug 24, 2018

wojta Aug 24, 2018

wtrocki commented Aug 24, 2018

wtrocki commented Aug 24, 2018

Correct message when all datasources filtered out #112

Correct message when all datasources filtered out #112

Conversation

wojta commented Aug 17, 2018 • edited

Motivation

What

Why

Verification Steps

Checklist:

Progress

Additional Notes

cfoskin left a comment

Choose a reason for hiding this comment

josemigallas commented Aug 23, 2018

Choose a reason for hiding this comment

wojta Aug 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Aug 24, 2018

Choose a reason for hiding this comment

wojta Aug 24, 2018 • edited

Choose a reason for hiding this comment

josemigallas Aug 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtrocki commented Aug 24, 2018

wtrocki commented Aug 24, 2018

wojta commented Aug 17, 2018 •

edited

wojta Aug 23, 2018 •

edited

wojta Aug 24, 2018 •

edited

josemigallas Aug 24, 2018 •

edited