Skip to content

Conversation

aluanhaddad
Copy link
Contributor

@aluanhaddad aluanhaddad commented Feb 13, 2018

This PR:

Describes the configuration object that can be passed to customize the bindable decorator and the BindableProperty class with a new interface BindablePropertyConfig

It goes on to

  • used this interface type where appropriate

  • fix a few minor typos (spelling) in the surrounding text

@aluanhaddad aluanhaddad force-pushed the patch-1 branch 2 times, most recently from 506d704 to f72deb0 Compare February 13, 2018 00:22
@aluanhaddad
Copy link
Contributor Author

aluanhaddad commented Feb 13, 2018

I just realized this overlaps with #560, albeit in largely non-conflicting way (type vs interface).

@StrahilKazlachev
Copy link
Contributor

@aluanhaddad can you exclude the dist, or did you add it to illustrate something?
Also the other and this PR are missing primaryProperty?: boolean(attributes only) in the config.
@bigopon what do you think about adding the typings now? Or is #560 close to being complete?

@bigopon
Copy link
Member

bigopon commented Feb 13, 2018

It's @EisenbergEffect 's / @jdanyow 's call, the other PR will take sometime to land

/**
* An optional interface describing the configuration object that can be specified to customize bindable properties.
*/
interface BindablePropertyConfig {
Copy link
Member

Choose a reason for hiding this comment

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

it also takes a property name defaultValue, type any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

 * describes the configuration object that can be passed to customize
the bindable decorator and the BindableProperty class

 * used the interface type appropriate

 * fixed a few minor typos (spelling)

 * built and ran tests (+1 squashed commits)
@aluanhaddad
Copy link
Contributor Author

aluanhaddad commented Feb 13, 2018

Thanks @bigopon and @StrahilKazlachev!

I have made the requested changes.

I didn't know about either attribute. primaryProperty looks awesome, super useful.

I definitely understand if ya'll want to wait on #560, but these type changes could help a lot discoverability.
The reason I created this PR was because I had forgotten for the umpteenth time (my memory isn't great 😁) if the property that specifies the binding mode was mode, bindingMode, or defaultBindingMode.

Copy link
Contributor

@EisenbergEffect EisenbergEffect left a comment

Choose a reason for hiding this comment

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

Just one minor change and we can get it in after that.

* @param aliases The array of aliases to associate to the custom attribute.
*/
export function customAttribute(name: string, defaultBindingMode?: number, aliases?: string[]): any {
export function customAttribute(name: string, defaultBindingMode?: bindingMode, aliases?: string[]): any {
Copy link
Contributor

Choose a reason for hiding this comment

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

bindingMode is unfortunately a const object at this time. So, I don't think it can be used as a type. Can we revert this back to number?

Copy link
Member

Choose a reason for hiding this comment

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

Note that with newest API on BindableProperty, it now understands lookup property of bindingMode such as toView, fromView, twoView, oneTime

@EisenbergEffect do we need to adjust this based on that ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bigopon So, it will accept either a string or a number?

Copy link
Member

Choose a reason for hiding this comment

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

@EisenbergEffect Yes it's added in this commit e7da973#diff-8fecf3f03eea2e940742fe22c0791bfbR51

It was to support Aurelia plugins / classes

/**
* The default binding mode of the property.
*/
defaultBindingMode?: bindingMode;
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 this will need to be number, since bindingMode is actually an object, not an enum or union type.

Choose a reason for hiding this comment

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

It's not that simple.
From usage it should be union type.
But I can't see how to make it possible

@EisenbergEffect
Copy link
Contributor

EisenbergEffect commented Jul 5, 2018 via email

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.

5 participants