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

fix(datepicker): add a style for the current mode #1312

Closed
wants to merge 1 commit into from
Closed

fix(datepicker): add a style for the current mode #1312

wants to merge 1 commit into from

Conversation

scamden
Copy link
Contributor

@scamden scamden commented Nov 27, 2013

This PR adds a style name to the table to reflect the current mode. This allows user to override the style for each mode. In particular my use case was to adjust the widths to be equal for all modes and apply a round border radius only in day mode.

@bekos
Copy link
Contributor

bekos commented Nov 27, 2013

@scamden I understand your case, but this way you hard code the css class into the javascript file. IMO, a cleaner approach is to just expose the mode variable into the scope and let the developer handle it on the template anyway he likes.

There is #924 that tries to do this, but I must review it again in order to land.

@scamden
Copy link
Contributor Author

scamden commented Nov 28, 2013

I agree having the class in js is ugly, how about putting the mode or mode name on the scope and then still putting a class that uses that scope variable on the template? I tried to choose a class that doesn't have any inherent style for bootstrap so it should have no effect but at least the dev has some ability to select on mode and override without having to modify the template. 


Sent from Mailbox for iPhone

On Wed, Nov 27, 2013 at 2:21 AM, Tasos Bekos notifications@github.com
wrote:

@scamden I understand your case, but this way you hard code the css class into the javascript file. IMO, a cleaner approach is to just expose the mode variable into the scope and let the developer handle it on the template anyway he likes.

There is #924 that tries to do this, but I must review it again in order to land.

Reply to this email directly or view it on GitHub:
#1312 (comment)

@bekos
Copy link
Contributor

bekos commented Nov 29, 2013

I think the more general and definite solution to this problem is to make mode a two-way binded variable. This way user can initialize / change mode programmatically and also will be able to something like:

<datepicker ng-class="my-dapicker-{{mode}}"></datepicker>

I will look into this when I find time to work on #924, unless you can help with that :-)

@pkozlowski-opensource
Copy link
Member

@scamden @bekos my impression is that we've got this discussion since we've got one big template for all the modes. This is why @scamden is afraid of overriding it. I'm really not big fan of introducing many options and people should be afraid of overriding templates. We are going into great deal of pain to make those templates customizable so it is a pity when people are unwilling to use this to their advantage.

@scamden
Copy link
Contributor Author

scamden commented Dec 2, 2013

@pkozlowski-opensource i'm not afraid to override the template. in fact, i have overridden both modal window and backdrop because i had to. the reason i prefer not to is maintenance. overriding a complicated template is usually a copy and paste kinda deal which has been beaten into me as a basically evil act because later when i update the library i have to try to manually merge in any changes and ensure that my template still works with the updates. in the case of modal, the templates were simple enough that i wasn't too worried about that.

in this case, even if i override the template there's no way (as far as i can see) to reference the mode. i'm totally down to help on the refactor @bekos if you direct me in terms of what needs to be finished and the proper way to get your code (i'm still a bit new to the open source thing).

in the meantime though, my change is only 2 lines (apart from tests), doesn't break anything or prevent that refactor and allows the user at least some hook to modify the style based on modes. i'm happy to move the "-mode" portion to the template if that feels cleaner.

@bekos
Copy link
Contributor

bekos commented Jan 20, 2014

@scamden Sorry for the late response and thx for your will to help with this :-)

Closing in favor of #1516 that seems like the proper solution for me. This way you will be able to define the datepicker mode class in the wrapper div of your datepicker and apply your custom styles. Mode will also be available in the template if you are willing to override it.

Thx again for your time 👍

@bekos bekos closed this Jan 20, 2014
@scamden
Copy link
Contributor Author

scamden commented Jan 20, 2014

no worries, this does what i need thanks for the help

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