Skip to content
This repository was archived by the owner on May 29, 2019. It is now read-only.

bs.config or ui.config #10

Closed
ProLoser opened this issue Oct 24, 2012 · 32 comments
Closed

bs.config or ui.config #10

ProLoser opened this issue Oct 24, 2012 · 32 comments

Comments

@ProLoser
Copy link
Member

I am surprised you guys haven't already indoctrinated this practice yet because I think it's one of the best parts of AngularUI.

Any thoughts to adding this so that you can do app-wide configurations?

@pkozlowski-opensource
Copy link
Member

Sure, having a config shared by a certain directive instances is a much needed thing, we should add it to the modal for example.

Now, what I kind of don't like in the current angular-ui approach is that we've got a global namespace. Couldn't we have config on the directive-type basis? Sth like:

angular.module('myModule', ['bs.modal']).constant('bs.modal': {fade:true, backdroop:true}); ?

@petebacondarwin
Copy link
Member

Do you mean "constant"? Constants can only be set in config blocks, like
providers. It would be more flexible to have it as a "value"

angular.module('myModule', ['bs.modal']).value('bs.modal': {fade:true,
backdroop:true});

Also do we need to worry about bs.modal being used for the constant/value
and the directive names?

Pete

On 24 October 2012 21:50, Pawel Kozlowski [email protected] wrote:

Sure, having a config shared by a certain directive instances is a much
needed thing, we should add it to the modal for example.

Now, what I kind of don't like in the current angular-ui approach is that
we've got a global namespace. Couldn't we have config on the directive-type
basis? Sth like:

angular.module('myModule', ['bs.modal']).constant('bs.modal': {fade:true,
backdroop:true}); ?


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-9755981.

@pkozlowski-opensource
Copy link
Member

@petebacondarwin yep, I've meant constant (grr, I'm almost sure I've added this to the example...).

Regarding the variable - sure it is good as well. Constants got the advantage of being "injectable" in any block (so config and run). But sure, variables would work as well.

Now, for the naming - no, those don't have to be the same. We could have:

angular.module('myModule', ['bs.modal']).constant('bs.modal.config': {fade:true, backdroop:true});

or whatever other convention we choose to support.

@petebacondarwin
Copy link
Member

Oh yes. Of course: the constant itself cannot be swapped out but the
fields on it can be changed anywhere right. Also it doesn't have the
overhead of potentially being decorated.

On 24 October 2012 21:59, Pawel Kozlowski [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin yep, I've meant
constant (grr, I'm almost sure I've added this to the example...).

Regarding the variable - sure it is good as well. Constants got the
advantage of being "injectable" in any block (so config and run). But sure,
variables would work as well.

Now, for the naming - no, those don't have to be the same. We could have:

angular.module('myModule', ['bs.modal']).constant('bs.modal.config':
{fade:true, backdroop:true});

or whatever other convention we choose to support.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-9756371.

@pkozlowski-opensource
Copy link
Member

@petebacondarwin exactly :-) While we can define a constant only once we can swap object properties anywhere. And you can define those constants on your app module.

@ProLoser
Copy link
Member Author

There is a growing schism in the architecture between bootstrap and ui that
I think we need to sort out soon. I discussed this briefly with pawel.
On Oct 24, 2012 2:23 PM, "Pawel Kozlowski" [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin exactly :-) While
we can define a constant only once we can swap object properties anywhere.
And you can define those constants on your app module.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-9757284.

@pkozlowski-opensource
Copy link
Member

Fully agree that we need to discus! Had a chat with Dean on IRC and I
think that we really should organize a hangout to discuss this. From
what I've understood Dean was really busy with his project at work but
we should organize a hangout as soon as he is available.

I'm definitively against diverging even further, we need to sync both
projects. At the same time we need to learn from the angular-ui
experiences (both good and bad).

@petebacondarwin
Copy link
Member

So a constant, then, for each module that needs one that is defined by the library and is updated by the user of the library in a config or run section if they want different defaults?

@petebacondarwin
Copy link
Member

And perhaps it should be named [component name].config, e.g. "$dialog.config" or "accordion.config"?

@pkozlowski-opensource
Copy link
Member

@petebacondarwin this would be probably the simplest solution. At the same time it should be transparent for users that don't care about changing defaults as in this case there is not need to define dependencies on any special modules.

I would say +1 for this.

@lanterndev
Copy link
Contributor

Sounds good to me! Thanks!

@petebacondarwin
Copy link
Member

If there are no complaints then I will knock up a PR for this tonight.

@pkozlowski-opensource
Copy link
Member

@petebacondarwin awesome! Just one remark: maybe those constants should follow the usual naming convention for constants, that is: upper-case and underscores as words separators. So ACCORDION_CONFIG for example.

WDYT?

@petebacondarwin
Copy link
Member

Actually I wonder if constant is not really the right idea so I am not sure
about all caps. It is really global/default config. We are actually
suggesting that people override it with their own config.

How about accordionConfig?

On 21 January 2013 18:02, Pawel Kozlowski [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin awesome! Just one
remark: maybe those constants should follow the usual naming convention for
constants, that is: upper-case and underscores as words separators. So
ACCORDION_CONFIG for example.

WDYT?


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-12509655.

@pkozlowski-opensource
Copy link
Member

Hmm, my idea was that we suggesting that people can override values of this constant and not the constant itself. This way people don't need to define anything, just inject the config constant and change whatever values they feel like changing. But I don't have strong opinion here. I like constant since I can inject them in both config and run blocks as well as in services etc.

@petebacondarwin
Copy link
Member

Yes sure the object itself is not going to change but given that you can
change its properties it is not really a constant in my mind. Capitalized
identifiers for me are constant primitives that ate immutable.

... sent from my tablet
On 21 Jan 2013 18:15, "Pawel Kozlowski" [email protected] wrote:

Hmm, my idea was that we suggesting that people can override values of
this constant and not the constant itself. This way people don't need to
define anything, just inject the config constant and change whatever values
they feel like changing. But I don't have strong opinion here. I like
constant since I can inject them in both config and run blocks as well as
in services etc.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-12510185.

@pkozlowski-opensource
Copy link
Member

OK, whatever then, as long as we are talking about AngularJS constants :-)

@pkozlowski-opensource
Copy link
Member

So, after the 4ea002b landed we've got a pattern for directives config. We should stick to this approach unless there is strong objection / better idea. If not let's use this ticket to add defaults to other directives.

@lanterndev
Copy link
Contributor

Looks great to me! I can try to work on a PR today to accept configuration for modal and tooltip, unless you want to claim these.

@pkozlowski-opensource
Copy link
Member

@Skivvies, go for it!

@ProLoser
Copy link
Member Author

No name spacing needed?
On Jan 27, 2013 9:57 AM, "Pawel Kozlowski" [email protected] wrote:

@Skivvies https://github.com/skivvies, go for it!


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-12758159.

@pkozlowski-opensource
Copy link
Member

@ProLoser Good point... Just one remark - would avoid a namespace with a dot, since it is a pain in a &@^%@! if you don't minify files.

@ProLoser
Copy link
Member Author

You mean our 'ui.config' -> 'uiConfig'? Lol. I am surprised you guys have
gotten this far into the thread without deciding to name space I figured
that there was a reason.
On Jan 27, 2013 11:53 AM, "Pawel Kozlowski" [email protected]
wrote:

@ProLoser https://github.com/ProLoser Good point... Just one remark -
would avoid a namespace with a dot, since it is a pain in a &@^%@! if you
don't minify files.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-12760138.

@ProLoser
Copy link
Member Author

I know I have occasionally forgot but I don't know of a better convention.
Plus aren't you still using dots?
On Jan 27, 2013 11:55 AM, "Dean Sofer" [email protected] wrote:

You mean our 'ui.config' -> 'uiConfig'? Lol. I am surprised you guys have
gotten this far into the thread without deciding to name space I figured
that there was a reason.
On Jan 27, 2013 11:53 AM, "Pawel Kozlowski" [email protected]
wrote:

@ProLoser https://github.com/ProLoser Good point... Just one remark -
would avoid a namespace with a dot, since it is a pain in a &@^%@! if you
don't minify files.


Reply to this email directly or view it on GitHubhttps://github.com//issues/10#issuecomment-12760138.

@ajoslin
Copy link
Contributor

ajoslin commented Jan 28, 2013

I made it responsive earlier, but now it's just messing stuff up. We should probably just remove bootstrap-responsive.js and the mobile meta tags from the header.

@zshamrock
Copy link

Hi, guys. We also need the way to configure template location for the components. We use angular bootstrap for spring application, and just templateUrl: template/pagination/pagination.html (for example), doesn't work. Right now I've just used the ui.config approach and update the directive directly, but then I need to maintain my own version of angular bootstrap. Also I will redo it using constants for configuration (I like the opportunity to inject it into the config method, because my templatePath config depends on another constant applicationContext). I will send the pull request this week, just to have the code in place to discuss.

@pkozlowski-opensource
Copy link
Member

@zshamrock actually the recommended approach here would be to bundle templates and stick them into $templateCache. This is what we are doing when building a distribution files with the -tpls in their names, more info about different build files here: https://github.com/angular-ui/bootstrap/tree/gh-pages#build-files and more info about partials preloading here: http://stackoverflow.com/a/12346901/1418796

Having said the above I recognize that for development if might be useful to download partials on-the-fly. I've opened the issue #105 to look into this.

Out of curiosity: are you customizing default bootstrap templates?

@lanterndev
Copy link
Contributor

Hey guys, I've been holding off on the PR for modal and tooltip config since @ProLoser raised the namespace issue to wait for that to get resolved, but I can go ahead and just match 4ea002b for now if you think you'd accept it? I'd love to be able to move to angular-ui bootstrap and I think that's the only thing stopping me!

@pkozlowski-opensource
Copy link
Member

@Skivvies I guess resolving issues around projects organization, prefixes etc. will take a bi of time so feel free to send a PR mimicking what was done in 4ea002b.

@lanterndev
Copy link
Contributor

Thanks @pkozlowski-opensource, just submitted #127 for the modal config. Since updating to the latest I'm having issues with tooltip again. Will try to create a plunker and link to it from #78. Once that's resolved I can submit another PR for tooltip config.

@pkozlowski-opensource
Copy link
Member

Closing this issue for now as many of the directives got their config options already. We should be adding systematically global config options for new directives and add them to existing directives as requested.

@lanterndev
Copy link
Contributor

Hi @pkozlowski-opensource, thought I'd give angular ui bootstrap's tooltip another shot as of the 0.2.0 release, and still haven't figured out how to specify tooltip-animation="true" globally. I'm guessing it's possible though. Seems like a much more common use case to want to specify this globally rather than per element, but http://angular-ui.github.com/bootstrap/#/tooltip only covers the per-element case.

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

No branches or pull requests

6 participants