Skip to content
This repository was archived by the owner on Aug 7, 2020. It is now read-only.

feat(oui-select): allow items to be disabled #291

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

marie-j
Copy link
Contributor

@marie-j marie-j commented Oct 12, 2018

Allow items from oui-select to be disabled

Copy link
Contributor

@AxelPeter AxelPeter left a comment

Choose a reason for hiding this comment

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

Nice job :)
Some improvements are needed (documentation and checks). Do you have some unit tests too for this feature ?

@@ -17,6 +17,7 @@ export default () => ({
title: "@?",
placeholder: "@?",
items: "<",
disabledItems: "<",
Copy link
Contributor

Choose a reason for hiding this comment

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

This property should be optional: <?
Don't forget to update the API table too :)

data-title="Select a country"
placeholder="Select a country..."
items="$ctrl.countries"
disabled-items="['Algeria', 'Canada', 'France', 'United States']"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you should add a note under/above these examples about the fact that disabled-items need an array of values based on the match :)

Since match is optional, how will work this property in the case of the property is not defined ?
This should be documented too.

@@ -29,6 +32,14 @@ export default class {
matchElement.html("{{$select.selected}}");
}

if (isArray(this.disabledItems)) {
this.items = angular.copy(this.items).map(item =>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use another property like this.sortedItems to keep the value of this.item.
What will happened when items or disabled-items is updated after the init ?

@@ -29,6 +32,14 @@ export default class {
matchElement.html("{{$select.selected}}");
}

if (isArray(this.disabledItems)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in $onInit instead :)

@marie-j marie-j force-pushed the feature/oui-select-disabled-options branch from 0c951a1 to 4d464fc Compare October 16, 2018 15:40
@@ -17,6 +17,7 @@ export default class {
$onInit () {
addBooleanParameter(this, "disabled");
addBooleanParameter(this, "required");

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this blank space :)

@AxelPeter AxelPeter merged commit fbc565b into develop Oct 19, 2018
@AxelPeter AxelPeter deleted the feature/oui-select-disabled-options branch October 19, 2018 13:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants