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

Setting the width through a property js #460

Merged
merged 4 commits into from
May 3, 2016

Conversation

negherbon
Copy link
Contributor

@negherbon negherbon commented May 2, 2016

Sometimes is useful put specific width through js property because dont need put any css class every single time you need a specific width. I believe this can help us.

Readme:

captura de tela 2016-05-03 as 08 48 07

#222

@faceleg
Copy link
Contributor

faceleg commented May 2, 2016

I don't understand why you've put a reference to #222 here?

@faceleg
Copy link
Contributor

faceleg commented May 2, 2016

Although I personally would prefer to use classes, as I tend to want more differences than just width, I can understand this might help some users.

Additional changes required for merge:

  • Update readme
  • Check if width is string, if it is use value without 'px', for people who may want to specify width in a different unit

@faceleg
Copy link
Contributor

faceleg commented May 2, 2016

You can use https://docs.angularjs.org/api/ng/function/angular.isString for the string check

@negherbon
Copy link
Contributor Author

@faceleg thanks for answer, i put #222 reference because has a talk about how to set width and height. But you are right i need create one issue for that, i will close this pull request and create one issue first.

Thanks :)

@negherbon negherbon closed this May 2, 2016
@faceleg
Copy link
Contributor

faceleg commented May 2, 2016

No that's fine, keep this PR open, I was just confused about the reference is all :)

@negherbon negherbon reopened this May 2, 2016
@negherbon
Copy link
Contributor Author

@faceleg ok, i will make the adjustments :)

@negherbon
Copy link
Contributor Author

@faceleg done o/


This option allows you to control the dialog's width. Default value is `null` (unspecified)

If you provide a number 'px' metric will be used, on the other hand you are able to provide a specific metric using String like `'40%'`
Copy link
Contributor

Choose a reason for hiding this comment

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

If you provide a Number, 'px' will be appended. To use a custom metric, use a String, e.g. '40%'.

@faceleg
Copy link
Contributor

faceleg commented May 3, 2016

Added some comments, overall looks GREAT!

@faceleg
Copy link
Contributor

faceleg commented May 3, 2016

This line: https://github.com/negherbon/ngDialog/blob/width-property/js/ngDialog.js#L548 needs to be expanded into a multi-line conditional please, then I think you're done

@negherbon
Copy link
Contributor Author

@faceleg refactoring done, thanks

@faceleg faceleg merged commit 2b15adc into likeastore:master May 3, 2016
@faceleg
Copy link
Contributor

faceleg commented May 3, 2016

Thanks a lot, especially for your attention to detail and patience.

@negherbon
Copy link
Contributor Author

@faceleg, thank you for helping me, it was my first contribution and i'm very happy. :)

@faceleg
Copy link
Contributor

faceleg commented May 3, 2016

You did great!

Don't be afraid to argue your case if you disagree with a suggestion by a maintainer :)

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.

2 participants