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

feat(accordion): add config service to provide default values #655

Closed
wants to merge 1 commit into from

Conversation

jnizet
Copy link
Member

@jnizet jnizet commented Sep 1, 2016

refs #518

@@ -23,18 +23,18 @@ export class NgbProgressbar {
/**
* Maximal value to be displayed in the progressbar.
*/
@Input() max = 100;
@Input() max: number;

Choose a reason for hiding this comment

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

If we do this then we are loosing default values info from the docs, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. The api-docs component now gets the default value here, and if undefined, looks for it in the matching property of the associated config service. So the default value should in fact not be defined here:

  • it's useless code, since it's immediately overwritten by the constructor, which copies the default value from the property
  • it's misleading code, since it makes the reader think that the default value is 100, whereas it's in fact copied from the config property in the constructor
  • it's also misleading because the generated documentation reads the default value from here although the actual default value comes from the config property

The last point makes me realize that the api-docs component should do the opposite of what it does now: try to find the default value in the matching config property, and only if not found, read it from here.

I'll amend the PR.

@pkozlowski-opensource
Copy link
Member

@jnizet could you please rebase? I think it has a conflict with your other PR I've just merged :-) Thnx!

@jnizet
Copy link
Member Author

jnizet commented Sep 2, 2016

@pkozlowski-opensource rebase done

* Returns the default value of the given directive input. If falsy, returns the default value of the matching
* config service property
* Returns the default value of the given directive input by first looking for it in the matching config service
* property. If no matching config property, reads it from the input.
Copy link
Member

Choose a reason for hiding this comment

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

"If there is no..., it reads it from..."

@icfantv
Copy link
Member

icfantv commented Sep 2, 2016

@jnizet just the one grammar change and LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants