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

Fixed invisible configuration values in backend #4085

Merged
merged 5 commits into from
Jul 15, 2024

Conversation

fballiano
Copy link
Contributor

@fballiano fballiano commented Jul 6, 2024

The issue is perfectly explained by @ADDISON74 in #3583 (comment)

Instead of re-adding the text-shadow back I would change the color of the disabled items to #202856, which is the same color used for the other items in the configuration pages.

what do you think?

screenshot after this PR applied:
Screenshot 2024-07-06 alle 10 29 58

@github-actions github-actions bot added Template : admin Relates to admin template Component: Adminhtml Relates to Mage_Adminhtml labels Jul 6, 2024
@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 6, 2024

In the legacy theme, the background color of an option is rgb(206, 206, 206) = #CECECE. For choosing the right text color we have to use a shades generator like this one (I set it up to 16 shades)

https://mdigi.tools/color-shades/#cecece

From #080808 to #787878 and making a few tests I think #585858 is the one. This is how it looks like

screenshot

@fballiano
Copy link
Contributor Author

good catch, what do you think about #999?

Screenshot 2024-07-07 alle 20 56 01

I tried it only because it was already used in the CSS and it's usually better not to have too many colors.

I've done it for the legacy and the new theme now if that's ok for you

@ADDISON74
Copy link
Contributor

#999 is used in CSS, but here we have to refer to the elements we want to change. The background and color of the text. We clearly do not change the background and the color of the text must be related to the background in order to achieve a pleasing differentiation for the eyes. This way we can get rid of the addition of the shadow.

You have to choose a value in that range of shades. If you compare my image with yours, you will feel a balance of colors in mine. I have no merit in this, I just followed one of the rules for choosing color pallets.

kiatng
kiatng previously approved these changes Jul 8, 2024
Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

Agree to reuse existing color.

@ADDISON74
Copy link
Contributor

Agree to reuse existing color.

If this statement will be a rule, then it means that we could use any other colors that can be found in the CSS file. My opinion remains unchanged, on a page the colors to be added to certain HTML elements are either the existing ones on the page, shades of the existing ones, or new ones in the color palette (adjacent, complementary, triadic, ...).

@fballiano
Copy link
Contributor Author

when we talk about important elements, usually it's good to have as few colors as possible, one for text, one for disabled text, one for primary buttons etc, that's more or less what all css frameworks do and it's done for easiness of understanding from the brain.

IMHO the color you proposed is a bit too visible to be a disabled color, imho it kinda doesn't seem disabled to me.

@ADDISON74
Copy link
Contributor

IMHO the color you proposed is a bit too visible to be a disabled color, imho it kinda doesn't seem disabled to me.

I proposed a color arguing how I chose it, by reference to the UI rules. Based on that rule, the color wanted by the majority could be reached, if the one chosen by me is not suitable.

@ADDISON74
Copy link
Contributor

If you still want to reuse an existing color, then I propose to use the color of the text that does not have a background, in the images bellow, the one in the "First Day of Week" option, which is #969696.

This is how it looks before

before

This is how it looks after

after

@fballiano
Copy link
Contributor Author

maybe it's a browser/OS difference because I used exactly that element to generate the 999, and in my browser I see

Screenshot 2024-07-08 alle 22 07 03

and 153,153,153 gets converted to 999

Screenshot 2024-07-08 alle 22 08 26

😱 I don't know why we're seing different things

@ADDISON74
Copy link
Contributor

ADDISON74 commented Jul 9, 2024

I am using Chrome/Windows 11.

select:disabled {
color: light-dark (graytext, rgb(170, 170, 170));
}

It is "user agent stylesheet". Using the color picker the color value is "#969696".

@fballiano
Copy link
Contributor Author

ok, weird that they're different, anyway in the legacy css, if i'm not wrong, there's no 969696, while there are multple 999

Copy link
Contributor

@ADDISON74 ADDISON74 left a comment

Choose a reason for hiding this comment

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

It will remain #999, although I would have wanted a shade relative to the background color.

@fballiano fballiano merged commit 6ccaabf into OpenMage:main Jul 15, 2024
3 checks passed
@fballiano fballiano deleted the textshadowproblem branch July 15, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants