Buttons parameter to array #534

Open
wants to merge 8 commits into
from

Projects

None yet

3 participants

@DavidePastore
DavidePastore commented Aug 19, 2016 edited

This will change the type of options.buttons parameter as array. This will help us to choose the order of the buttons. Now you can set the key of the button using the key property.

Before:

bootbox.dialog({
  message: "I am a custom dialog",
  title: "Custom title",
  buttons: {
    success: {
      label: "Success!",
      className: "btn-success",
      callback: function() {
        Example.show("great success");
      }
    },
    danger: {
      label: "Danger!",
      className: "btn-danger",
      callback: function() {
        Example.show("uh oh, look out!");
      }
    },
    main: {
      label: "Click ME!",
      className: "btn-primary",
      callback: function() {
        Example.show("Primary button");
      }
    }
  }
});

With this PR:

bootbox.dialog({
  message: "I am a custom dialog",
  title: "Custom title",
  buttons: [
    {
      key: "success",
      label: "Success!",
      className: "btn-success",
      callback: function() {
        Example.show("great success");
      }
    },
    {
      key: "danger",
      label: "Danger!",
      className: "btn-danger",
      callback: function() {
        Example.show("uh oh, look out!");
      }
    },
    {
      key: "main",
      label: "Click ME!",
      className: "btn-primary",
      callback: function() {
        Example.show("Primary button");
      }
    }
  ]
});

Related issues: #291, #293, #431 and #525.

Things to do:

  • WIP (#535) Update documentation
  • Create a migration guide
@tarlepp tarlepp and 1 other commented on an outdated diff Aug 19, 2016
@@ -216,6 +216,24 @@
* merge a set of default dialog options with user supplied arguments
*/
function mergeArguments(defaults, args, properties) {
+ var mapArgumentsResult = mapArguments(
+ args,
+ properties
+ );
+ if(defaults && defaults.buttons && mapArgumentsResult && mapArgumentsResult.buttons){
@tarlepp
tarlepp Aug 19, 2016 edited Collaborator

code style is different than anywhere else. There is a ton of these "problems".

@DavidePastore
DavidePastore Aug 19, 2016

Thanks for reporting the issue. Using the grunt command it's all fine (apart from the getKeyLength function). What are you referring to when you talk about code style?

@tarlepp
tarlepp Aug 19, 2016 Collaborator

you're using eg. if(...){ and everywhere else bootbox uses if (...) {

@DavidePastore
DavidePastore Aug 19, 2016 edited

Oh sorry, you're right! Is there a way to auto fix the code, so I use the same standard? Maybe a linter or something like this. I suppose it should be an automatic (grunt?) task.

@tarlepp
Collaborator
tarlepp commented Aug 19, 2016

Also there should be some migration guide to convert old code for this. Or maybe just some serious notes is enough - although also docs needs to update.

@tarlepp tarlepp and 1 other commented on an outdated diff Aug 19, 2016
buttons[key] = {
label: _t(value)
};
+ */
@tarlepp
tarlepp Aug 19, 2016 Collaborator

dead code?

@DavidePastore
DavidePastore Aug 19, 2016

You're right. 😄

@tarlepp tarlepp commented on an outdated diff Aug 19, 2016
if ($.isFunction(options.callback)) {
return options.callback.call(this);
}
return true;
};
+ /**
+ * overrides
+ */
+ each(options.buttons, function(buttonKey, button){
@tarlepp
tarlepp Aug 19, 2016 Collaborator

another code style issue here use function(...) {

@tarlepp tarlepp and 1 other commented on an outdated diff Aug 19, 2016
@@ -216,6 +207,24 @@
* merge a set of default dialog options with user supplied arguments
*/
function mergeArguments(defaults, args, properties) {
+ var mapArgumentsResult = mapArguments(
+ args,
+ properties
+ );
+ if (defaults && defaults.buttons && mapArgumentsResult && mapArgumentsResult.buttons) {
@tarlepp
tarlepp Aug 19, 2016 Collaborator

personally I like that code is readable, so I would add empty lines on these.

);
if (...)

to

);

if (...)
@DavidePastore
DavidePastore Aug 19, 2016

I agree with you. Is there any other place where you find the same issue?

@tarlepp
tarlepp Aug 19, 2016 Collaborator

Didn't look closer, you should spot those quite easily.

@tarlepp tarlepp commented on an outdated diff Aug 19, 2016
@@ -216,6 +207,25 @@
* merge a set of default dialog options with user supplied arguments
*/
function mergeArguments(defaults, args, properties) {
+ var mapArgumentsResult = mapArguments(
+ args,
+ properties
+ );
+
+ if (defaults && defaults.buttons && mapArgumentsResult && mapArgumentsResult.buttons) {
+ $.each(defaults.buttons, function(defaultButtonKey, defaultButton) {
+ var found = false;
+ $.each(mapArgumentsResult.buttons, function(mapArgumentButtonKey, mapArgumentButton) {
@tarlepp
tarlepp Aug 19, 2016 Collaborator

empty line for more readable code.

@tarlepp tarlepp commented on an outdated diff Aug 19, 2016
@@ -216,6 +207,25 @@
* merge a set of default dialog options with user supplied arguments
*/
function mergeArguments(defaults, args, properties) {
+ var mapArgumentsResult = mapArguments(
+ args,
+ properties
+ );
+
+ if (defaults && defaults.buttons && mapArgumentsResult && mapArgumentsResult.buttons) {
+ $.each(defaults.buttons, function(defaultButtonKey, defaultButton) {
+ var found = false;
+ $.each(mapArgumentsResult.buttons, function(mapArgumentButtonKey, mapArgumentButton) {
+ if (defaultButton.key === mapArgumentButton.key) {
+ found = true;
+ }
+ });
+ if (!found) {
@tarlepp
tarlepp Aug 19, 2016 Collaborator

empty line for more readable code.

@tarlepp
Collaborator

no, not here... I give up :D

@DavidePastore

@tarlepp Sorry If I misunderstand the magic line. 😢 Do you think the review is ok?

This was referenced Aug 21, 2016
@makeusabrew
Owner

See my comment in #291. Need to remember why this changed, but it changed for a good reason.

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