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

configure background and foreground color for custom fields #1552

Closed
wants to merge 6 commits into from

Conversation

Copy link
Member

@atrol atrol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fcaspar for your contribution

Some general remarks, some more following inline

  • conflicts with latest code have to be resolved
  • missing documentation in Admin Guide
  • why do you use color_background / color_foreground instead of the usual background_color / foreground_color?
  • the colors are not applied to the View Issues page, maybe even more (didn't check)
  • we use different colors for field labels and field values. After your change, they are the same.
    Not sure, if this should be changed (use the same color or add two more options)
  • usage of inline styles
    Inline styles should be avoided.
    At the moment we allow them, but it will not work any longer if CSP headers should be changed back to what we had in Mantis 1.3
    https://mantisbt.org/bugs/view.php?id=21908
    Would be good to get some feedback of other devs if introducing some more inline styles should be allowed

admin/schema.php Outdated Show resolved Hide resolved
admin/schema.php Outdated Show resolved Hide resolved
core/custom_field_api.php Outdated Show resolved Hide resolved
lang/strings_german.txt Outdated Show resolved Hide resolved
admin/schema.php Outdated Show resolved Hide resolved
schema update changed to 7 char null and correct table name
@fcaspar
Copy link
Author

fcaspar commented Sep 1, 2019

Thanks @fcaspar for your contribution

Thanks for reviewing. ;)

* conflicts with latest code have to be resolved

Is that a task for me? Could you please clarify?

* missing documentation in Admin Guide

Please recheck other points. If they are fine, I'll update documentation.

* why do you use color_background / color_foreground instead of the usual background_color / 

Changed.

* the colors are not applied to the `View Issues` page, maybe even more (didn't check)

I edited only bug_view_inc to consider the colors. Did I miss something?

* we use different colors for field labels and field values. After your change, they are the same.

Correct. Open point.

* usage of inline styles
  Inline styles should be avoided.

Inline Styles: I have no other idea how to affect a field's style individually. Do you have a suggestion?

@atrol
Copy link
Member

atrol commented Sep 8, 2019

It seems something that went wrong with your latest changes.
E.g. there are changes in lang/strings_english.txt and manage_custom_field_edit_page.php that are not related to the new functionality.

Please make sure that your submissions adhere to our Coding Guidelines http://www.mantisbt.org/wiki/doku.php/mantisbt:coding_guidelines
E.g. add a space before closing braces

* conflicts with latest code have to be resolved

Is that a task for me? Could you please clarify?

Typically yes.

* why do you use color_background / color_foreground instead of the usual background_color / 

Changed.

Not changed in schema.php

* the colors are not applied to the `View Issues` page, maybe even more (didn't check)

I edited only bug_view_inc to consider the colors. Did I miss something?

Custom fields can also be displayed on "View Issues" page if they are included via columns configuration.

  Inline styles should be avoided.

Inline Styles: I have no other idea how to affect a field's style individually. Do you have a suggestion?

It could be implemented similar to the status color, but I am not sure if it's worth the effort.
That's why I hoped to get some feedback of other devs.

@dregad @vboctor what's your opinion concerning usage of inline styles ?
At the moment we allow them, but it will not work any longer if CSP headers should be changed back to what we had in Mantis 1.3
https://mantisbt.org/bugs/view.php?id=21908

@syncguru
Copy link
Contributor

syncguru commented Sep 9, 2019

-1 - I am strongly against this change for the following reasons:

  1. Why treating custom fields any different from default ones. This seems arbitrary and not articulated at all
  2. UX consistency is as important as code if not more important. Allowing any random color in the page will clash with the ACE theme and impact UX. ACE comes with a set of compatible colors that we can use (we use across the product) - the selection is less than 10 colors.
  3. No inline CSS - please
  4. Storing colors in the DB?! Really??

@atrol
Copy link
Member

atrol commented Sep 9, 2019

-1 - I am strongly against this change

@syncguru I am also not 100% convinced that the current approach of the PR is what should be included in standard.
But strongly against?

Why treating custom fields any different from default ones.

Right, in best case default fields are nothing more than preconfigured custom fields.
But current Mantis is quite far away from this approach.

Allowing any random color in the page will clash with the ACE theme and impact UX.
ACE comes with a set of compatible colors that we can use (we use across the product) - the selection is less than 10 colors.

We allow random colors for status squares.

We can't prevent users from changing whatever they want.
I have seen JavaScript plugins and other tweaks that lead to an ugly Mantis 2.x in my opinion, but there are users that like it.

Keep also in mind that there are users that don't like the ACE theme / colors due to various reasons.
e.g. it violates https://www.w3.org/TR/WCAG20/ concerning contrast ratio
https://mantisbt.org/bugs/view.php?id=22253

So in best case we offer different themes and some more predefined colors to choose for other purposes, e.g. coloring of (custom) fields.

No inline CSS - please

+1 that's why I opened https://mantisbt.org/bugs/view.php?id=21908
Search the code for "style=" and "<style>>. You will find the dev who introduced most of the inline styles in 2.x ;-)

Storing colors in the DB?! Really??

It's not that unusual in Mantis. We allow storing status colors in configuration file and DB.

Copy link
Member

@vboctor vboctor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 from me as well.

As a goal, it would be good to reduce the gap between features of custom vs. native fields. For example, interleaving and ordering of native and custom fields, making a native field required, etc. Hopefully overtime, we will get there.

As for UX, we should limit the degrees of freedom customizations promise to enable control over the full UX. For example, in tools like Slack/Teams, a color can be assigned to a message, but it doesn't change foreground and background image, but it shows a bar next to the message to control the experience, similar to our status color approach. This way, UX can change overtime, themes can change (e.g. dark mode vs. standard mode), etc.

@atrol
Copy link
Member

atrol commented Nov 1, 2019

Closed based on feedback of other devs.
@fcaspar sorry, but you have to maintain your own fork / branch to keep this functionality.

For other users that might think about using these changes on their own Mantis instance:
There are database schema changes that conflict with those ones in standard Mantis (admin/schema.php), so you will get problems when trying to upgrade Mantis.

@atrol atrol closed this Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants