Fix form validation when adding machine with multiple MACs. #963
Conversation
| <label for="mac-address" class="p-form__label col-2" ng-class="{ 'is-required': mac === machine.macs[0] }"> | ||
| <span data-ng-if="mac === machine.macs[0]">MAC Address</span> | ||
| <label for="mac-address-{$ $index $}" class="p-form__label col-2" ng-class="{ 'is-required': mac === machine.macs[0] }"> | ||
| <span data-ng-if="mac === machine.macs[0]">MAC Address</span> |
There was a problem hiding this comment.
perhaps I'm misunderstanding this, but won't this make an empty <label> for the second and subsequent macs?
also instead of checking for === the first machine.macs can't we just use $first? (re: https://docs.angularjs.org/api/ng/directive/ngRepeat )
There was a problem hiding this comment.
I agree I don't think we should have this conditional in the label content. It looks like it just determines if the field should be required or not.
There was a problem hiding this comment.
perhaps I'm misunderstanding this, but won't this make an empty for the second and subsequent macs?
That's exactly what it does!
also instead of checking for === the first machine.macs can't we just use $first? (re: https://docs.angularjs.org/api/ng/directive/ngRepeat )
That's definitely the smarter way to do this. Generally with the angular pages I try to only change the things that are actually causing bugs (e.g. the input validation, not the labels), because often fixing/improving the current code leads to completely unrelated things breaking. This seems pretty innocuous though.
| <label for="mac-address" class="p-form__label col-2" ng-class="{ 'is-required': mac === machine.macs[0] }"> | ||
| <span data-ng-if="mac === machine.macs[0]">MAC Address</span> | ||
| <label for="mac-address-{$ $index $}" class="p-form__label col-2" ng-class="{ 'is-required': mac === machine.macs[0] }"> | ||
| <span data-ng-if="mac === machine.macs[0]">MAC Address</span> |
There was a problem hiding this comment.
I agree I don't think we should have this conditional in the label content. It looks like it just determines if the field should be required or not.
anthonydillon
added
Review: Code +1 (with changes)
Review: QA +1
and removed
Review: Code needed
Review: QA needed
labels
Caleb-Ellis
force-pushed
the
multiple-macs
branch
from
0149847 to
bf8431c
Compare
Done
QA
Fixes
Fixes #905