Add openClass and closeClass attributes #221

nlarche · 2017-02-24T11:28:26Z

Hello Guy,

I tried to do something, tell me If it's good to you and if you want me to do thing in another way
closes #219

regards,

Nicolas

coveralls · 2017-02-24T11:31:11Z

Coverage increased (+0.9%) to 74.444% when pulling b370617 on nlarche:master into f515c70 on guylabs:master.

coveralls · 2017-02-24T14:12:18Z

Coverage increased (+0.9%) to 74.444% when pulling 41940df on nlarche:master into f515c70 on guylabs:master.

guylabs

Hi @nlarche

first I must say best pull request since the project started! You added documentation, tests and increased the overall coverage! ;) really nice.

I added some comments, mainly formatting issues and documentation changes. Can you please check that all formatting changes are reverted as then you exactly see what changed in functionality/features.

And could you also execute a grunt dist and then update the pull request? Then all the distribution files are also updated.

When you manage to fix these things over the weekend then I can add them to the 0.4.0 release as I want to release this on Monday evening.

Thanks a lot! Great contribution!

Regards

Guy

guylabs · 2017-02-24T13:59:25Z

README.md

@@ -39,6 +39,7 @@ ion-autocomplete
        - [Manage externally](#manage-externally)
        - [Selected items](#selected-items)
        - [Clear on select](#clear-on-select)
+        - [Override open and close class ](#open-close-class)


Can you maybe change it to - [Open and close CSS class ](#open-and-close-css-class)?

guylabs · 2017-02-24T14:04:00Z

README.md

@@ -560,6 +561,14 @@ This option is to clear the search input when an item is selected. You need to s
 <input ion-autocomplete type="text" class="ion-autocomplete" autocomplete="off" ng-model="model" clear-on-select="true" />
 ```

+### Override open and close class


Please use the following documentation (just some minor changes):

Open and close CSS class

By default two CSS classes are used to display and hide the modal, namely the ion-autocomplete-open and the ion-autocomplete-close CSS class. These are used to show and hide the modal. When you need to override these classes, you can define the following two properties with your CSS class.

<input ion-autocomplete type="text" class="ion-autocomplete" open-class="my-open-class" close-class="my->close-class" />

guylabs · 2017-02-24T14:05:06Z

src/ion-autocomplete.css

@@ -5,7 +5,7 @@
    bottom: 0;
    left: 0;
    z-index: 20;
-    display: none;
+    /*display: none;*/


Please delete it right away as commented out code is something you don't need ;).

guylabs · 2017-02-24T14:05:31Z

src/ion-autocomplete.js

@@ -21,7 +21,7 @@ angular.module('ion-autocomplete', []).directive('ionAutocomplete', [
                selectedItemsLabel: '@',
                templateUrl: '@',
                itemValueKey: '@',
-                itemViewValueKey: '@'
+                itemViewValueKey: '@'              


Please remove the spaces you added at the end here.

guylabs · 2017-02-24T14:07:13Z

src/ion-autocomplete.js

@@ -477,7 +483,7 @@ angular.module('ion-autocomplete', []).directive('ionAutocomplete', [
                    // set the model value of the model
                    ngModelController.$parsers.push(function (viewValue) {
                        return ionAutocompleteController.getItemValue(viewValue, ionAutocompleteController.itemValueKey);
-                    });
+                    });                  


Please remove the spaces you added at the end here.

guylabs · 2017-02-24T14:08:03Z

test/e2e/ion-autocomplete.single-select.e2e.html

@@ -8,47 +8,60 @@
    <title>ion-autocomplete end to end test page</title>

    <link rel="stylesheet" href="../../bower_components/ionic/css/ionic.css">
-    <link rel="stylesheet" type="text/css" href="../../src/ion-autocomplete.css"/>
+    <link rel="stylesheet" type="text/css" href="../../src/ion-autocomplete.css" />


Please revert the formatting as this adds unneeded changes to the pull request.

guylabs · 2017-02-24T14:08:33Z

test/e2e/ion-autocomplete.single-select.e2e.html


    <script src="../../bower_components/ionic/js/ionic.bundle.js"></script>
    <script src="../../src/ion-autocomplete.js"></script>
    <script type="text/javascript">
        var app = angular.module('ion-autocomplete-sample', ['ionic', 'ion-autocomplete'])
-                .controller('IonAutocompleteController', function ($scope) {


Same here: Please revert the formatting as this adds unneeded changes to the pull request. (for the whole block from line 17 to 42)

guylabs · 2017-02-24T14:09:25Z

test/e2e/ion-autocomplete.single-select.e2e.html

-<ion-view title="ion-autocomplete sample">
-    <ion-content>
-        <form>
+    <ion-view title="ion-autocomplete sample">


Please revert the formatting as this adds unneeded changes to the pull request. Please check the rest of the file as there are more formatting changes like this.

nlarche · 2017-02-24T14:43:29Z

I made all changes and push dist files. it should be good.

coveralls · 2017-02-24T14:45:36Z

Coverage increased (+0.9%) to 74.444% when pulling bd63cd9 on nlarche:master into f515c70 on guylabs:master.

guylabs · 2017-02-24T15:28:39Z

@nlarche Did you run the local e2e tests with protractor? Looks like the master is now failing: https://travis-ci.org/guylabs/ion-autocomplete/builds/204997944 Could you maybe have a look at it and check if you are able to reproduce it locally?

The strange thing is that here in the pull request it said the build was passing and the coverage increased. I am able to have a look at it on Monday evening if you cannot check it.

Thanks and regards,

Guy

nlarche · 2017-02-24T16:21:55Z

@guylabs yes it's weird, I ran all the test in local. I can't check until Monday morning.

guylabs · 2017-02-25T10:22:43Z

@nlarche I just fixed it as the open/close classes for the e2e tests did not work properly. I changed them an now all the tests work properly. Can you maybe check it on Monday such that I can release 0.4.0 in the evening? That would be great!

Thanks a lot and have a good weekend.

nlarche · 2017-02-27T08:13:24Z

Ok, it looks good to me.

guylabs · 2017-02-27T09:48:18Z

Hey @nlarche, 0.4.0 is released. Thanks again for your contirbution!

Regards

Guy

Add openClass and closeClass attributes

b370617

Update ion-autocomplete.css

41940df

guylabs requested changes Feb 24, 2017

View reviewed changes

nilar added 4 commits February 24, 2017 15:37

remove uneeded changes

13b0164

minor changes

dd2d1fe

update ditribution files

8951134

Merge branch 'master' of https://github.com/nlarche/ion-autocomplete

bd63cd9

guylabs approved these changes Feb 24, 2017

View reviewed changes

guylabs self-assigned this Feb 24, 2017

guylabs added this to the 0.4.0 milestone Feb 24, 2017

guylabs merged commit cd062ac into guylabs:master Feb 24, 2017

guylabs pushed a commit that referenced this pull request Feb 25, 2017

#221(fix): adapt open/close class to support e2e tests

654d162

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add openClass and closeClass attributes #221

Add openClass and closeClass attributes #221

nlarche commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

guylabs left a comment

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

guylabs Feb 24, 2017

nlarche commented Feb 24, 2017

coveralls commented Feb 24, 2017

guylabs commented Feb 24, 2017

nlarche commented Feb 24, 2017

guylabs commented Feb 25, 2017

nlarche commented Feb 27, 2017

guylabs commented Feb 27, 2017

Add openClass and closeClass attributes #221

Add openClass and closeClass attributes #221

Conversation

nlarche commented Feb 24, 2017

coveralls commented Feb 24, 2017

coveralls commented Feb 24, 2017

guylabs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Open and close CSS class

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

nlarche commented Feb 24, 2017

coveralls commented Feb 24, 2017

guylabs commented Feb 24, 2017

nlarche commented Feb 24, 2017

guylabs commented Feb 25, 2017

nlarche commented Feb 27, 2017

guylabs commented Feb 27, 2017