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

Proposal: add semantic classes to datepicker directive (and possibly others as well) #2428

Closed
TiddoLangerak opened this issue Jul 7, 2014 · 6 comments

Comments

@TiddoLangerak
Copy link

I recently wanted to style the datepicker using custom css. Unfortunately I found that styling the datepicker cannot be done cleanly. Since the datepicker only has default bootstrap classes assigned to elements there are 2 options for styling the datepicker:

  • Copy-paste the default template and change classes in the copy
  • Override default styles using non-semantic selectors (e.g. [datepicker-popup-wrap] table .btn to target the calendar buttons).

The first option is fragile because it requires manually keeping the copied template up-to-date with upstream, and the second option is fragile since it gives little control and it has a very high coupling with the html structure, which again might be changed upstream.

Common practice is to include semantic class names in the html, such that these can be targeted very precisely using css only (proper separation of concerns). This technique should be applied to this directive as well.

This problem is likely not limited to the datepicker alone, but I simply have not checked the other directives.

@pkozlowski-opensource
Copy link
Member

@TiddoLangerak I see your point but in my view overriding default templates is the best curse of action. I'm not big fan of "semantic classes" as:

  • there is a risk of colliding with the CSS classes people might have defined in their projects,
  • it makes templates / library bigger and penalises people who don't need this feature
  • people will start asking for more classes on more elements (ex. buttons of date-picker) and it only makes the mentioned problems worse.
    So I wouldn't like to go down this path.

I think that targeting specific elements ( datepicker-popup-wrap in your example) is a reasonable compromise if you don't want to override templates.

Thoughts?

@TiddoLangerak
Copy link
Author

  • there is a risk of colliding with the CSS classes people might have defined in their projects,

Common practice to prevent this is to use a vendor prefix, just like with module names for angular. In this case we could use ui-* class names. This minimizes the risk of collisions. In the rare case that there are still collisions users can always target the date picker specifically by using more specific selectors.

  • it makes templates / library bigger and penalises people who don't need this feature

Although this is true, I think this is definitely worth it. I think that about 90% of the (professional) users of this library want to customize the look and feel of the date picker, but only a very small percentage needs to customize the html. The website also advertises "A clean, flexible and fully customizable date picker.". Adding proper CSS classes vastly improves the customizability and the size overhead is minimal. (In case size is critical users can still override the templates)

As a side note, adding classes might make the library bigger but it will improve rendering speed. Long selectors can have a significant performance impact on large pages since matching those elements is much harder for a browser. In real world scenarios the performance impact due to a slightly larger library will probably be smaller than the rendering penalty due to longer selectors[citation needed]. This is especially true when the templates are properly cached after initial load.

  • people will start asking for more classes on more elements (ex. buttons of date-picker) and it only makes the mentioned problems worse. So I wouldn't like to go down this path.

I fully agree that it should not be overdone, but that's in my opinion not a reason to not use classes. With a well designed set of classes you don't need many to allow the user to style everything.

I think that targeting specific elements ( datepicker-popup-wrap in your example) is a reasonable compromise if you don't want to override templates.

Targeting specific elements is generally considered to be an anti-pattern. CSS best practice dictates to use classes for styling. The main reason for this is separation of concerns: when targeting specific elements such as [datepicker-popup-wrap] you're assigning multiple responsibilities to the datepicker-popup-wrap attribute: it serves both as an entry point for functionality and as an entry point for layout. If at one point in time you decide to change the functional implementation (e.g. to improve performance) you now also need to take layout into account. A similar argument can be made for using element selectors, or even the default bootstrap classes (e.g. consider when Bootstrap releases a new version with different class names like they did with the 2->3 upgrade. This breaks any styling that depends on the bootstrap class names). This is at best a quick hack, but not a proper solution to style the datepicker.

@mg1075
Copy link

mg1075 commented Aug 21, 2014

+1

It would be nice if there were clear CSS classname hooks for customizing the datepicker.

I do not believe ui-* would be an appropriate vendor prefix, though,
since the CSS for jquery-ui has already adopted the ui- prefix.

http://api.jqueryui.com/theming/css-framework/

ui-ng-* : too verbose?
ang-* : too general?
aui-*: ?

And I think if someone is serious about using this datepicker,
they are going to want to immediately begin customizing the styles.
For instance, on the Datepicker demo page, the selected date
is in blue text on a blue background, making it barely legible.

Using @TiddoLangerak's 2nd suggestion,
revising the default might look like this:

[datepicker-popup-wrap] .btn-info.active,
[datepicker-popup-wrap] .btn-info.active .text-info {
    color: #ffffff;
}

@danielbsig
Copy link

+1 for semantic class names. At the very least a few carefully chosen elements could be chosen, and then developers could style the rest using CSS child selectors. There is still some risk of breaking that CSS if the markup structure changes in future versions, but at least those changes could be isolated within these carefully selected elements.

@brbradford
Copy link

+1

@wesleycho
Copy link
Contributor

Forgot to add in commit - this is addressed by 97c4333

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

No branches or pull requests

7 participants