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

zz Customization #4

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ziaulrehman40
Copy link

No description provided.

* Change positioning logic for non-inline usage
* Add support for ui-select dropdown to facilitate projects using ui-select as standard dropdown
* nextYeras, prevYears, hideTodayDate, useAngularUiSelect options added
* Some style enhancements
* Other minor adjustments
@ziaulrehman40 ziaulrehman40 changed the title Feature/zz customizations zz Customization Feb 6, 2017
…al cases

* Update bower and package main files to also have css.
* Stop propogation of click on selection of month or year, to prevent closing of the non-inline popup.
…al cases

* Update bower and package main files to also have css.
* Stop propogation of click on selection of month or year, to prevent closing of the non-inline popup.
…l hide/show calender

* Properly handle clicks on dropdowns to prevent closing calender.
@ziaulrehman40
Copy link
Author

We are still not finished with customizations/fixes on this. I will keep committing to this branch and will let you know when finished.

* Initiate datepicker after pageload(i.e in timeout(0), this is used to solve many problems which were arising because of initialization before complete page load.
* altTarget and toggleTarget can be passed as raw string selectors, which are resolved after page load. This solves the problem when we don't have rightly evaluated selectors before page load.
* Fixes and enhancements to documentclick handler, should solve all issues related to clicks in dropdowns(it has some todo's)
* Pure JQuery dependency is added, .is and .has functions of JQUery were needed and we were short on time to find pure JS or JQlite alternatives for these JQuery functions. Removing this dependency is in TODO's.
* Better detection of toggleTarget with in altTarget, if it is so, than click on toggletarget will not trigger altTarget click.
'
@ziaulrehman40
Copy link
Author

In last commit, to solve certain problems, i had to use jquery's .has and .is methods which are not available in jqlite. Because of which i had to add jquery 1.6+ dependency.
It will be great if you can remove these dependencies, also another small TODO is in code(that is not a big issue though, just not the right approach and it causing problems chance is very low).
I think these two issues should be solved, than this branch is good to be merged.

@vickramravichandran
Copy link
Owner

Great work with all the fixes and features. However, I cannot merge this directly to master due to the level of changes. I have created a dev branch with some refactoring and features that is not available in master. Can you re-submit a pull request, against the dev branch, with the following options? It will be easier to test and merge with master.

  1. hideTodayDate (true/false) - Default false for backward compatibility.
  2. prevYears
  3. nextYears
  4. updateTargetIfEmptyOrInvalid (true/false) - Renamed from setCurrentDateWhenEmpty. Default true for backward compatibility.
  5. altTargetToggle (true/false). If this option is true altTarget should toggle visibility. This way you can get rid of toggleTarget option completely. If you have a need to have both altTarget and toggleTarget, you can invoke the toggleShow() method externally. See ready callback option argument on how to get a reference to this method externally.

  1. toggleTarget - Not needed. See Refactoring and new features #5 above.
  2. useAngularUiSelect: I haven't looked into this yet. I will do so after the above changes are merged.

I have added the following options on the dev branch:

  1. todayDateFormat
  2. appendToBody (true/false)
  3. positionUsingJQuery (true/false)
  4. positionUsing

@ziaulrehman40
Copy link
Author

Let me create PR with dev as is now, if any updates are needed i will try to implement on weekend(week days are quite busy this week).
And about altTarget, as of my understanding, altTarget is for opening calender, while toggleTarget is added to toggleShow, difference is that altTarget will always show calender, while toggleTarget will toggle its display/hide state.
Please correct me if I am wrong.

@ziaulrehman40
Copy link
Author

Turns out, as dev is a new branch, github is not letting me create on that, see this SO post

Will take some time to sort this out from my end.

@vickramravichandran
Copy link
Owner

vickramravichandran commented Feb 14, 2017

Create a single commit with options 1 to 5. It will be easier to review that way. Next, you can create another commit with only the ui-select.

Yes, altTarget is for opening the calendar. I am trying to reuse altTarget to avoid having a toggleTarget specifically to toggle display/hide. The most common use case is to have a target (textbox) and an optional altTarget (image). If you really need the option to target an element as the toggleTarget you can do this:

  • If toggleTarget is true, altTarget will toggle the display/hide state.
  • If toggleToggle is an element or jQuery selector, that element will toggle the display/hide state.

@ziaulrehman40
Copy link
Author

ziaulrehman40 commented Feb 14, 2017

That's reasonable for toggleTarget to be true or a selector.

About a separate commit, can i ask what might be the reason to keep ui-select separate? If we are adding this option than why not let it be in same commit?
Actually this will save me some time that's why i am asking.
Nevertheless i will be able to work on this on weekend though.

Also by 5 options, does this also include all the fixes?

@vickramravichandran
Copy link
Owner

No hurry, weekend is fine. I prefer a separate commit for ui-select since it in itself is a big unique feature.

Did you try cloning 'dev' and creating a new branch from it?

@ziaulrehman40
Copy link
Author

Will do, weekend :)

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.

2 participants