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 missing "autocomplete", "placeholder" and "readonly"… #63

Merged
merged 10 commits into from
Sep 6, 2016

Conversation

lionel-bijaoui
Copy link
Member

Add missing "autocomplete", "placeholder" and "readonly" attr to input fields. Ordered alphabetically.

… attr to input fields. Ordered alphabetically.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.515% when pulling f5af70c on lionel-bijaoui:LB_autocomplete into 672e656 on icebob:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage remained the same at 86.515% when pulling f5af70c on lionel-bijaoui:LB_autocomplete into 672e656 on icebob:master.

@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage remained the same at 86.515% when pulling a7e39d9 on lionel-bijaoui:LB_autocomplete into 672e656 on icebob:master.

@coveralls
Copy link

coveralls commented Sep 1, 2016

Coverage Status

Coverage remained the same at 86.515% when pulling c645cea on lionel-bijaoui:LB_autocomplete into 672e656 on icebob:master.

@@ -1,5 +1,5 @@
<template lang="jade">
input.form-control(type="number", v-model="value", number, :min="schema.min", :max="schema.max", :readonly="schema.readonly", :disabled="disabled", :placeholder="schema.placeholder")
input.form-control(type="number", v-model="value", number, :autocomplete="schema.autocomplete", :disabled="disabled", :max="schema.max", :min="schema.min", :readonly="schema.readonly")
</template>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why removed the placeholder from here?

@coveralls
Copy link

coveralls commented Sep 2, 2016

Coverage Status

Coverage remained the same at 86.515% when pulling 79bfee6 on lionel-bijaoui:LB_autocomplete into 672e656 on icebob:master.

@icebob
Copy link
Member

icebob commented Sep 2, 2016

Please add tests to the new attributes like disabled attr.

@icebob icebob mentioned this pull request Sep 2, 2016
2 tasks
@icebob icebob added this to the v0.4.1 milestone Sep 2, 2016
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.515% when pulling 9ea47cc on lionel-bijaoui:LB_autocomplete into bccc910 on icebob:master.

1 similar comment
@coveralls
Copy link

coveralls commented Sep 2, 2016

Coverage Status

Coverage remained the same at 86.515% when pulling 9ea47cc on lionel-bijaoui:LB_autocomplete into bccc910 on icebob:master.

@lionel-bijaoui
Copy link
Member Author

lionel-bijaoui commented Sep 5, 2016

I had a little bit of hard time (dead computer 💀 ), but I was able to simplify the test for attributes to this:

import { createVueField, trigger, /*new thing in util.js --> */ attributesList } from "../util";

// other tests... 

describe.only("check optional attribute", () => {
            // here you name which attribute you want to test and that's it.
            let attributes = ["autocomplete", "disabled"];

            attributes.forEach(function(name) {
                it('should set ' + name, function(done) {
                    let schematic;
                    let attr = attributesList[name];

                    if (attr.field) {
                        schematic = field;
                    } else {
                        schematic = schema;
                    }
                    schematic[name] = attr.before;
                    vm.$nextTick(() => {
                        expect(input[name]).to.be.equal(schematic[name]);
                        schematic[name] = attr.after;
                        done();
                    });
                });
            });
        });
// util.js
export let attributesList = {
    "autocomplete": { before: "on", after: "off" },
    "disabled": { before: true, after: false, field: true },
    "placeholder": { before: "Field placeholder", after: "" },
    "readonly": { before: true, after: false }
    // etc...
};

@lionel-bijaoui
Copy link
Member Author

My tests fail if the properties are not set in the schema

@lionel-bijaoui
Copy link
Member Author

Woops my bad

@icebob
Copy link
Member

icebob commented Sep 5, 2016

My tests fail if the properties are not set in the schema

Please explain, I don't understand.

@lionel-bijaoui
Copy link
Member Author

lionel-bijaoui commented Sep 6, 2016

describe("check template", () => {
    let schema = {
        type: "masked",
        label: "Phone",
        model: "phone",
        // autocomplete: "off", // <-- not set here
        placeholder: "",
        readonly: false,
        cleaveOptions: {
            phone: true,
            phoneRegionCode: "HU",
        }
    };
    let model = { phone: "30 123 4567" };
    let input;

    before(() => {
        createField(this, schema, model, false);
        input = el.getElementsByTagName("input")[0];
    });

    it('should set autocomplete', function(done) {
        schema.autocomplete = "on";
        vm.$nextTick(() => {
            expect(input.autocomplete).to.be.equal(schema.autocomplete);  // <-- fail here
            schema.autocomplete = "off";
            done();
        });
    });
});

but:

describe("check template", () => {
    let schema = {
        type: "masked",
        label: "Phone",
        model: "phone",
        autocomplete: "off", // <-- set here
        placeholder: "",
        readonly: false,
        cleaveOptions: {
            phone: true,
            phoneRegionCode: "HU",
        }
    };
    let model = { phone: "30 123 4567" };
    let input;

    before(() => {
        createField(this, schema, model, false);
        input = el.getElementsByTagName("input")[0];
    });

    it('should set autocomplete', function(done) {
        schema.autocomplete = "on";
        vm.$nextTick(() => {
            expect(input.autocomplete).to.be.equal(schema.autocomplete);  // <-- work here
            schema.autocomplete = "off";
            done();
        });
    });
});

Why ?

@icebob
Copy link
Member

icebob commented Sep 6, 2016

Yes, because first case the "autocomplete" property is not reactive. Use schema.$set("autocomplete", "on") or field.schema.$set("autocomplete", "on")

@lionel-bijaoui
Copy link
Member Author

I tried it and it trigger other errors. I want to finish it so I'm going to do it by initializing for now.

expect(input.readOnly).to.be.false;
expect(input.disabled).to.be.false;
// expect(input.placeholder).to.be.equal(schema.placeholder);
// expect(input.readOnly).to.be.false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this old commented rows (other files too).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't sure you wanted to delete them or not, so meanwhile, I commented them. I will clean things now !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are mindful. But if we covered with your new code we can delete them. Because after 3 months, we won't know it is a skipped tests (what need to fix) or unneccessary :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, make sense, sorry about that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nop, if you ready, I will merge it. 🎉

@icebob
Copy link
Member

icebob commented Sep 6, 2016

So can be merge? (after green travis)

@icebob icebob merged commit f120d52 into vue-generators:master Sep 6, 2016
@lionel-bijaoui lionel-bijaoui deleted the LB_autocomplete branch September 6, 2016 16:41
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