Skip to content

Conversation

@krvajal
Copy link
Contributor

@krvajal krvajal commented Oct 10, 2017

This pull request allows the use of multiple class names when passing the className options to the button. The different classes should be separated by spaces.

This pull request allows the use of multiple class names when passing the `className` options to the button. The different classes should be separated by spaces.
@wutmikee
Copy link

wutmikee commented Nov 8, 2017

Is this getting pushed?

@pbarbiero
Copy link

pbarbiero commented Nov 17, 2017

+ buttonEl.classList.add(className);

Shouldnt that be

+ buttonEl.classList.add(className_);

Also, @t4t5 it would be great if we could get this feature added. It'd also be nice if we could exclude the swal--button class as well so we dont have to reset it if we are overhauling the button design.

@lionralfs
Copy link
Collaborator

Thanks for the PR, @krvajal

I went ahead and added the possibility to also pass an array of classNames, as discussed in #743,
as well as some tests.

Looks good @t4t5?

@krvajal krvajal changed the title [wip] Allow multiple class names on buttons Allow multiple class names on buttons Nov 18, 2017
Copy link
Contributor Author

@krvajal krvajal left a comment

Choose a reason for hiding this comment

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

Please take a look a the suggested changes, as I am not a maintainer of the project you are not required to follow them but it will be nice to do :)

buttonEl.classList.add(name);
});
} else if (Array.isArray(className)) {
className.forEach(name => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please check the length of the name here, and also DRY when adding the class names. In both cases, we get an array from the input, so you could instead do this

const classNameArray = Array.isArray(className)? className: className.split(' ')
classNameArray.forEach(name => {
buttonEl.classList.add(name);
});

Makes sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure, great idea.
Do you agree with this?

const classNameArray = Array.isArray(className) ? className : className.split(' ');
classNameArray
  .filter(name => name.length > 0)
  .forEach(name => {
    buttonEl.classList.add(name);
  });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first line can be split into parts for clarity if you want but yes, that's the idea.

@krvajal
Copy link
Contributor Author

krvajal commented Dec 4, 2017

@t4t5 are you considering adding this?

@bahirul
Copy link

bahirul commented Dec 9, 2017

please merge this ... we need this feature 👍

@t4t5
Copy link
Owner

t4t5 commented Dec 14, 2017

Looks great guys! Thanks for the work @krvajal @lionralfs!

@t4t5 t4t5 merged commit 250ddf2 into t4t5:master Dec 14, 2017
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.

6 participants