employee filter.asc.desc.load #18

zyahav · 2017-01-04T21:47:27Z

No description provided.

myok12 · 2017-01-05T00:40:40Z

ex9/phonebook.html

+<body>
+  <input type="button" id="btnReloadList" value="Reload" />
+  <input type="button" id="btnASC" value="ASC" />
+  <input type="button" id="btnDESC" value="DESC" hidden />


I didn't know there's a hidden attribute. But it seems fine.

myok12 · 2017-01-05T00:44:03Z

ex9/phonebook.html

+  <input type="button" id="btnDESC" value="DESC" hidden />
+
+  <input type="button" id="btnSearchOn" value="Search" />
+  <input type="text" id="textSearch" value="" hidden />


I would put all the search stuff into some container (<div>) so that I can hide it all in one action. It would also help to see the high-level features in your page.
You can then also give the container an ID/class (let's say "search") and rename the inner elements' ID, or might not need to give them IDs/classes at all as they are the only ones in this container (#search input { ... }).

myok12 · 2017-01-05T00:45:47Z

ex9/phonebook.html

+
+  <ol id="phoneBook">
+    <li>
+      <span class="name fname">


Now that you actually load the data dynamically, I'd remove the pre-filled values.

myok12 · 2017-01-05T00:49:36Z

ex9/js/main.js

+        }
+      },
+      searchChange: function() {
+        console.log('searchChange');


This repeats exactly what's above. It normally implies you need to refactor the code so there's no repetition.

myok12 · 2017-01-05T00:50:19Z

ex9/js/main.js

+      searchOn: function() {
+        console.log('searchOn');
+        // Search off
+        $( "#btnFilter" ).prop( "disabled", true );


I would call searchChange instead of setting disabled directly.
Also, since you are not clearing the input after you hide and re-show it, it probably starts with the last search then, but the button is disabled.

myok12 · 2017-01-05T00:52:05Z

ex9/js/main.js

+    var on;
+    setEvents();
+    on = {
+      loadList: function() {


This file is WAY too big and deal with three different things. Please split these into three files:

searchui

sortui

data (which will later also include the code to do the searching/sorting).

This will also make you think about what values you need from #1 and #2, and will make them more like objects.

myok12 · 2017-01-05T01:03:14Z

ex9/js/main.js

@@ -0,0 +1,95 @@
+require(['ajaxGet'], function(ajaxGet) {
+  $(function() {


I would actually inline (=move the code of something into here) setEvents into the $(function() {...}), but move the on outside. Also, I don't think the on object helps in any way, I'd vote to just its methods be top-level functions.

myok12 · 2017-01-06T21:10:45Z

ex9/js/order.js

@@ -0,0 +1,27 @@
+define(function() {
+  return function() {


You can just return setEvents; instead of this anonymous function

myok12 · 2017-01-06T21:14:52Z

ex9/js/search.js

+
+  function showSearch() {
+    console.log('searchOn');
+    $( '#btnFilter' ).prop( "disabled", true );


I would call onFilterChange instead.

myok12 · 2017-01-06T21:15:16Z

ex9/js/search.js

+  function showSearch() {
+    console.log('searchOn');
+    $( '#btnFilter' ).prop( "disabled", true );
+    $('#filter').val('Type your filter');


Use this instead: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#attr-placeholder

myok12 · 2017-01-08T16:42:34Z

ex9/js/ajaxGet.js

+        url: url,
+        statusCode: {
+          404: function() {
+            alert( "page not found" );


I wouldn't do this error specific approach. There are MANY errors that can occur. The error callback is enough

myok12 · 2017-01-08T16:44:10Z

ex9/js/ajaxGet.js

+        },
+        error: function(fff){
+            alert( "it is an error" + fff );
+            console.log(fff);


Once you understand what is the argument, rename fff to it. Also, the doc for error says what this param is. See http://api.jquery.com/jquery.ajax/

myok12 · 2017-01-08T23:18:38Z

ex9/js/main.js

@@ -0,0 +1,37 @@
+requirejs(['ajaxGet','load','search', 'order'] , function(ajaxGet, load, search, order) {
+  $(function() {
+    var fun = {


Again, you don't need this object for calling methods on. If you want this to hold the data, just rename it to data (or better, phoneBookData) and access it from the functions as needed.
For the methods that others (order, filter) need to call, you can just pass another object: return {sortByAsc: ...}

myok12 · 2017-01-08T23:20:08Z

ex9/js/main.js

+      },
+      filter: function() {
+          var filterList = _.filter(fun.data, function(name) {
+            if (name.name.indexOf($( '#filter' ).val()) > -1) return name;


filter expects your function to return true or false (http://underscorejs.org/#filter). You currently return name or undefined instead.

myok12 · 2017-01-08T23:21:01Z

ex9/js/main.js

+        );
+        fun.print( _.sortBy(filterList, 'name').reverse());
+      },
+      print: function(objList) {


print is usually for something like console.log. Use render instead.

myok12 · 2017-01-09T14:56:58Z

ex9/js/filter.js

+    });
+
+    $('#filter').on('keydown', function(val) {
+      if (val.which == 13) filter();


What happens if you press Enter but there's no input (val==='' as below)?
That's why you need all entry point to search to use the same code?

myok12 · 2017-01-09T14:58:30Z

ex9/js/main.js

+    };
+
+    load(function() {
+      ajaxGet('json/data.json', function(phoneBookDataA) {


phoneBookDataA is not too descriptive. How about phoneBookDataFromServer?

myok12 · 2017-01-09T14:59:14Z

ex9/js/main.js

+requirejs(['ajaxGet','load','filter', 'sort'] , function(ajaxGet, load, filter, sort) {
+  $(function() {
+    var phoneBookData = {};
+    var render = function(objList) {


var render = function(objList) is almost the same as function render(objList), except the latter cannot be modified. Use the latter form as possible.

Also, please put some comments as to what objList is. Similar to phoneBookData, it is an array of employees (technically a subset of phoneBookData).

myok12 · 2017-01-09T15:01:27Z

ex9/js/main.js

@@ -0,0 +1,38 @@
+requirejs(['ajaxGet','load','filter', 'sort'] , function(ajaxGet, load, filter, sort) {
+  $(function() {
+    var phoneBookData = {};


I think your code elsewhere expects to find an array and not an object (try filtering or ordering without loading the data -- you'll probably get an Error). Initialize to [] instead.

myok12 · 2017-01-09T15:03:49Z

ex9/js/main.js

+
+    filter(function() {
+      var filterList = _.filter(phoneBookData, function(name) {
+        if (name.name.indexOf($( '#filter' ).val()) > -1) return true;


Instead of here accessing the "internal" of the filter UI, have the caller (in filter.js) pass the filter value into this the function filter calls back (line 25 above).

myok12 · 2017-01-09T15:05:44Z

ex9/js/main.js

+      render( _.sortBy(filterList, 'name'));
+    });
+
+    sort(function() {


Since the sortAsc and sortDesc are similar, I'd make them use the same callback with some param passed (for example: sort({asc: true}); or sort({asc: false});.
This makes even more sense when there's more info you wish to convey to the callback, like in filter (jQuery use objects for almost everything, think $.ajax).

myok12 · 2017-01-09T15:06:42Z

ex9/js/filter.js

+    });
+
+    $('#btnMin').click(function() {
+      console.log('searchOff');


Clicking on this should undo the filter. Currently it only affects the filter UI.

myok12 · 2017-01-09T15:07:48Z

ex9/js/order.js

@@ -0,0 +1,17 @@
+define(function() {


This file isn't being used anymore. Delete.

rm order.js==>sort.js search.js==>filter.js
done

html + main

323af1f

myok12 reviewed Jan 5, 2017

View reviewed changes

html container and split files

c5c05b4

myok12 reviewed Jan 6, 2017

View reviewed changes

test commit

78e0e0a

myok12 reviewed Jan 8, 2017

View reviewed changes

zyahav added 3 commits January 9, 2017 00:37

css sort/filter/load

67abd0e

delete all calls to ajax stor data in obj

f28aa17

remove ajax

9c0df66

myok12 reviewed Jan 8, 2017

View reviewed changes

zyahav added 6 commits January 9, 2017 10:53

filter return true

660f246

rename print to render

00d6294

remove error 404

01c62a0

styl, remove li on click

1872632

more style

ba5bbe0

assad

572532c

myok12 reviewed Jan 9, 2017

View reviewed changes

zyahav added 2 commits January 10, 2017 02:19

cards

5f2f6e6

main, records

6f1d3f6

		@@ -0,0 +1,95 @@
		require(['ajaxGet'], function(ajaxGet) {
		$(function() {

employee filter.asc.desc.load #18

Are you sure you want to change the base?

employee filter.asc.desc.load #18

Uh oh!

Conversation

zyahav commented Jan 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants