-
Notifications
You must be signed in to change notification settings - Fork 117
WIP: smRadioGroup and smRadioButton directives #10
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Conversation
@caitp how does it look like so far ? |
'<sm-radio-button value="1">One</sm-radio-button>', | ||
'<sm-radio-button value="2">Two</sm-radio-button>', | ||
'<sm-radio-button value="3">Three</sm-radio-button>', | ||
'</sm-radio-group>'].join('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should probably just make a helper function that does this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where that helper function should be placed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caitp, can you make a refresh in here please. What sort of helper function did you mean ? and also my initial question is still needs an answer :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like
function createRadioGroup(model, valuesAndLabels) {
var template = '<sm-radio-group ng-model="' + model + '">' +
Object.getOwnPropertyNames(valuesAndLabels).map(function(value) {
var label = valuesAndLabels[value];
return '<sm-radio-button value="' + value + '">' + label + '</sm-radio-button>';
}).join("") + '</sm-radio-group>';
element = $compile(template)($scope);
// and whatever other setup you want to do
}
var kBaseKeys = Object.freeze({
1: "One",
2: "Two",
3: "Three"
});
createRadioGroup("value", kBaseKeys);
or something like that --- but it's your call man, it can just be a bit nicer to write tests without having to do lots of string concatenation and stuff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, Object.keys is probably a better option, so you avoid engine stuff creeping in... might be better to use arrays to keep the order. anyway, do whatever works for you
the implementation basically looks okay to me, room for a few improvements, but mostly just nits |
you can certainly merge this whenever you like, if you want to, adding helpers to the test suite to make it a bit easier to write tests is something you can do later (or not), if you like. LGTM |
I haven't added any accessibility stuff, that matters as I understood from the first directives, will add that part also and then good to go for merge 👍 |
+1 |
@caitp Can we land this and then improve the accessibility stuff ? I am finishing one project and hope to get more free time. |
yes, lgtm, merge at will =) |
…utton WIP: smRadioGroup and smRadioButton directives
Status: Work in progress
Based on #5