charettes / django-colorful

Extension to the Django web framework that provides database and form color fields
https://pypi.python.org/pypi/django-colorful
MIT License
169 stars 58 forks source link

Correct CSS typo and add display property to fix IE11 rendering issue #40

Closed sessionzero closed 6 years ago

sessionzero commented 7 years ago

Internet Explorer 11 doesn't support the color type input and makes use of the Really Simple Color Picker. However it doesn't display correctly due to a typo introduced in 68c54e49fcb6f785f4ef020e02495c9db18e2110 ('div' became 'v') and a lack of a display property.

Prior to change: ie11

After change: ie11_edited_css

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 2f9c631d2af32df5f66e94a8f1df5798e1fa22bf on sessionzero:master into 7b54bf9b758e6fd9aaaaf78819b5eb15e51ac5de on charettes:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 2f9c631d2af32df5f66e94a8f1df5798e1fa22bf on sessionzero:master into 7b54bf9b758e6fd9aaaaf78819b5eb15e51ac5de on charettes:master.

charettes commented 7 years ago

While I'm open to fix the v -> div typo I don't think stuffing display: inline-block; should be part of this part of this file as the change is going to be lost the next time we upgrade to the latest laktek/really-simple-color-picker version.

sessionzero commented 7 years ago

To ensure the display property isn't overwritten when updating the color picker it could be set through separate CSS file. Alternatively a comment could be added to the colorPicker.css file identifying the property's purpose, hopefully warning contributors of its importance. What would you prefer?

I also did a little more testing and noticed this problem isn't limited to Internet Explorer 11; potentially it affects all browsers without support for the color type input. I tried Firefox 28 (prior to support) with the same results as above.

charettes commented 7 years ago

@sessionzero is this something you could report upstream to the laktek/really-simple-color-picker project or does it only happen within the Django admin?

sessionzero commented 7 years ago

I believe it's specific to the Django admin page. The really-simple-color-picker leaves the layout up to implementer through separate CSS as per their demo.

charettes commented 7 years ago

Alright then.

Since the color widget might be used outside of the admin, where display: inline-block might not be appropriate, the correct solution would be to define a colorful.admin.AdminColorWidget(ColorWidget) class that includes an extra js media with the display override and have the admin use it instead which can be done in two ways.

  1. Document that every BaseModelAdmin subclasses must define formfield_overrides[ColorField] = {'widget': AdminColorWidget}.
  2. Define a colorful app (e.g. colorful.app.AdminColorful) with a ready() method that sets FORMFIELD_FOR_DBFIELD_DEFAULTS[ColorField] = {'widget': AdminColorWidget} and document that colorful.app.AdminColorful should be set in INSTALLED_APPS (instead of simply colorful) to get this behavior.