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

fix disabled button color #1184

Closed
wants to merge 1 commit into from
Closed

Conversation

the-j0k3r
Copy link
Member

@the-j0k3r the-j0k3r commented Jul 17, 2020

This is how a disabled button looks,

Capture

Looks anything BUT.

re: #1154 issue 2

The reason why I PR this is for @silverwind to see how using $color for buttons is less than ideal idea because whats good in one value for some property in a rule elsewhere isnt necessarily good for another rule with a different property, in this case this PR is good for disabled button text color but not good for other rules.

Read the diff and see the wack-a-mole game

I cant commit this in good conscience and manual fixes at this level is not something I want to revisit.

body[class="page-responsive"] .btn-primary .octicon {
color: hsla(0, 0%, 100%, .8);
color: hsla(0, 0%, 100%, .3);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here at this value the octicon is going to break because $color thinks this is OK. SO we fix one and break others.

Copy link
Member Author

@the-j0k3r the-j0k3r left a comment

Choose a reason for hiding this comment

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

So we see in order to fix the disabled buttons text color we will break other things, and @silvewind keeps adding $color everywhere :(

.btn-primary .octicon {
color: hsla(0, 0%, 100%, .8);
color: hsla(0, 0%, 100%, .3);
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

.section-codespaces-develop-night .codespaces-develop-text {
color: hsla(0, 0%, 100%, .8) !important;
color: hsla(0, 0%, 100%, .3) !important;
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

.flash-header {
color: hsla(0, 0%, 100%, .8);
color: hsla(0, 0%, 100%, .3);
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

html[class^="ghh"] .ghh button.ghh-aux {
color: hsla(0, 0%, 100%, .8);
color: hsla(0, 0%, 100%, .3);
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

@@ -34687,9 +34687,9 @@
.cpt-notification {
color: #fff;
}
/* githubstatus.com: "rgba(255,255,255,0.8)" */
/* githubstatus.com: "color: hsla(0,0%,100%,.8)" */
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

.layout-content.status.status-index .page-status .last-updated-stamp {
color: hsla(0, 0%, 100%, .8);
color: hsla(0, 0%, 100%, .3);
Copy link
Member Author

Choose a reason for hiding this comment

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

also will break.

@silverwind
Copy link
Member

The main issue here is that a inversion approach does not cleanly work because GH uses white text and we do too (ideally we should have black text but that won't work either). I see no good solution except to manually override the disabled button selectors unfortunately.

silverwind added a commit that referenced this pull request Jul 21, 2020
* fix dropdown arrows (silverwind)
* add manual rule for disabled buttons, fixes #1184 (silverwind)
@the-j0k3r
Copy link
Member Author

Ideally we wouldnt fix what isnt broken, buttons use to work and already manual styles, it seems like were going backwards here.

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

Successfully merging this pull request may close these issues.

2 participants