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

Fixed wrong param name dateFormat in datepickerPopupConfig #1810

Closed
wants to merge 4 commits into from

Conversation

rafbgarcia
Copy link
Contributor

Here it is

@Foxandxss
Copy link
Contributor

When submitting a PR you need to update (or add if needed) the tests and then run karma to check that everything is ok. The build did break.

@rafbgarcia
Copy link
Contributor Author

Sorry, I was doing something else and didn't realize that.

Done :-)

@bekos
Copy link
Contributor

bekos commented Feb 15, 2014

@rafbgarcia Seems that a test is missing for this config . Can you add one? It shouldn't be much trouble.

@rafbgarcia
Copy link
Contributor Author

I think it's ok now.

@bekos
Copy link
Contributor

bekos commented Feb 22, 2014

@rafbgarcia I don't think this is the correct test. What you test is already tested in the 'custom format' describe block.

You should add a test in the 'setting datepickerConfig', by doing datepickerConfig.datepickerPopup= '...'; Does this makes sense?

@rafbgarcia
Copy link
Contributor Author

@bekos I'm now giving full attention to it.
I read the code and in my understanding the this param is only available in datepickerPopup, right?
I can't find the tests to the constant datepickerPopupConfig, there is a test 'as popup' which also doesn't test it.
Should we add tests to it or there is a reason why this is not being tested?

@bekos
Copy link
Contributor

bekos commented Feb 22, 2014

@rafbgarcia Oops, my mistake. You are right, we are missing tests for the datepickerPopupConfig. Can you add them in a similar way that datepickerConfig is tested?

@rafbgarcia
Copy link
Contributor Author

@bekos what do you think now?

@bekos
Copy link
Contributor

bekos commented Feb 26, 2014

@rafbgarcia Nice 👍

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

Successfully merging this pull request may close these issues.

4 participants