Merge pull requests #64 and #81.

[nickl-]

Merge @koppor bootstrap3 #64 koppor/bootstrap3.
Merge @jugautam fontawesome #81 jugautam/master.
This should also resolve PRs #78 #76 and makes reference to open issues #82 #72 #62 #60.

If all parties can agree (@peteb4ker @juristr @panicoenlaxbox @jodytate @jbogdani) please close your individual Issues to regain some order around here in @sciactive's absence.

[juristr]

Did you run into issue regarding bootstrap3 support for which reason you have to distinguish between bootstrap and bootstrap3?? In my pull request #76 I just added additional bootstrap3 CSS classes where needed (https://github.com/sciactive/pnotify/pull/76/files), mainly 'cause I wanted to avoid duplicating the property definitions just for the sake of bootstrap versions.

[koppor]

Sounds like a good idea. Your commit d94e017, however, seems to be missing hi_bnd and hi_btn.

[juristr]

@koppor sure, might be that I missed those two classes.

[nickl-]

@juristr I was considering it as a project maintainer ( @sciactive ? everything ok ? ) would and weighing the good for the many against the good for the few. We might never look back at BS 1&2 I don't disagree but this is not the time nor the place. To reach a conclusion I took the following factors into account, community acceptance and involvement, adherence to current style and practices but mostly I favoured those that accomplished their task and nothing else.

You see my mug up there but the work is not mine and credit goes to the committers retained. My role was purely maintenance and my goal is to reduce the issue queue. This counts 8 open and related issues but since I am not a maintainer it is upto you whether you agree and show your support by closing your own issues in favour of joining forces towards a common goal.

The sooner we get this done the sooner we can move on to new things #87

[juristr]

@nickl- Absolutely, I do close my pull request without any issue. That's why I started commenting as I went through the commits of @jbogdani who submitted commit 90c4949 to solve #60 and then I noticed that he added a bootstrap3 configuration other than augmenting the existing bootstrap configuration as mentioned initially in #60. As a consequence you have to explicitly specify whether to use bootstrap 2 or bootstrap 3.

His commit is perfectly valid..as mentioned, my only objection was the need for that explicit bootstrap declaration. 😄 I'll close my pull request in favor of solving the upgrade issue with this one here.

[nickl-]

I agree with you that bootstrap 3 should be the new default as it is also the default when visiting http://getbootstrap.com

I have added you as a committer to the fork so that you can update the default to bootstrap 3.

Once pushed it should update this pull request automatically.

If you want to suggest more improvements rather make a branch and submit additional pull requests for review so as not to complicate this topic.

[juristr]

Well, what I meant is not necessarily that it should be the default as there might be people that are still stick with v2. Rather I meant that as a user (if possible and I think it should be) you shouldn't have to specify explicitly which version.
In my commit it should have worked with both, bootstrap 2 as well as with bootstrap 3. The overhead you have is that you have "useless" bootstrap v2 classes on your DOM if your project uses bootstrap 3 and vice versa.

[nickl-]

@juristr there are 3 points in your argument which I would like to raise different arguments for but I do agree with your general notion that it should be as simple as possible for new users and be an optimum solution.

As much as we aim for things to remain easy for users we also want maintenance to be simple, am I correct?

The 3 points in bullet form:

• Default v2 or v3
  As mentioned before the default for bootstrap is v3 what I didn't stress is that it has been for the best part of a year. If the only reason you will stay on v2 is because of other projects dependencies then you may be relieved to know that his project is one amongst very few popular bootstrap compatible projects which hasn't been updated already.

Going bootstrap 3 will cause BC breaks and therefor should likely accompany a major version bump as indicative.

• Combining bootstrap as one option
  From what I can tell the two versions are quote different and and it is not that simple to detect a different version due to the nature of CSS. If we need to identify the difference why not define them separately from the start? This will simplify maintenance when bugs are fixed in the previous version and will also simplify the eventual removal of the outdated support.
• Useless members assigned to the DOM due to duplicate definition.
  I believe this is a separate issue again and not something to complicate this commit as the same argument is valid for jqueryui vs bootstrap since you will only ever use one.

Changing the assignment of the style property from a hard coded collection to perhaps a switch statement will solve the problem and only assign the values as per the default style type, but I do think this will be marginal at best.

What do you think?

[juristr]

As much as we aim for things to remain easy for users we also want maintenance to be simple, am I correct?

👍

Default v2 or v3
dsf
Default is for sure v3, no doubt. The only reason why people might (have to) remain at v2 is - as mentioned - for older projects where there is no need to upgrade. And yes, you cannot simply upgrade, it will cause a BC.

...not that simple to detect a different version due to the nature of CSS. If we need to identify the difference why not define them separately from the start?...

Do we actually need to identify from within the plugin which bootstrap version is being used?? I started from the assumption that if we take the bootstrap v2 compatible pnotify configuration and augment it with the new bootstrap v3 classes we should have done it. (Ignore my statement if I miss something here as I didn't dive too much into the code)

Changing the assignment of the style property from a hard coded collection to perhaps a switch statement

Nah...I wouldn't do that. Switch statements are normally against having maintainable code ;)

As mentioned, I'm also fine in using different bootstrap configurations. As you correctly say, from a maintenance point of view it might be easier afterwards (at some point) to drop old bootstrap v2 stuff once it is no more used. It's a bit of a matter of taste and preferences. Adding a separate bootstrap v3 configuration is a bit uglier from the end -user point of view (but it's a relative, small overhead...probably not even worth discussing 😄 ). On the other it is preferable from a maintenance point of view..

I am fine with both approaches.

[mariuszkerl]

Waiting for bootstrap3 changes please put them on nuget asap :) 👍

[hperrin]

I'm going through pull requests and merging them. Is this ready to merge? I haven't looked at it much.

[jacqueslareau]

Just add a bootstrap3 class and merge please. Such a simple issue, everybody is using their own custom version to support it.

[koppor]

I followed @juristr's suggestion in my commit 2bb772e. Tested with bootstrap3. @nickl-: I didn't add a minified version to avoid conflicts with the other merges. Please go ahead :)

[nickl-]

@hperrin Ready to merge and solves #64 #81 #78 #76 and very likely #72 #62 #60 as well.

Glad you're back! =)

[hperrin]

Mergy mergy merge.

[koppor]

@hperrin Thanx. Do you vote for a separate configuration for bootstrap2/bootstrap3 or for a one-size-fits all solution. If you opt for the latter, Pls merge #64 to include commit 2bb772e :)

[hperrin]

I'd rather not have all the classes included on the icon element. If someone wants to use that class, it could clash. Besides, if someone is using Bootstrap 3, it's just one extra line of code.