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

Darkly tweak #1811

Merged
merged 8 commits into from
Jul 7, 2023
Merged

Darkly tweak #1811

merged 8 commits into from
Jul 7, 2023

Conversation

SleeplessOne1917
Copy link
Member

Description

Make outline buttons have more contrast in darkly themes

Screenshots

Before

image

After

image

Comment on lines 60 to 62
.btn-outline-secondary {
color: $gray-500;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If there's a way to do this by setting an scss variable, let me know. That is preferable to needing to put this selector in every darkly scss file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think there may be; one sec.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is, I think, a consequence of the way we're doing these theme colors, the concern I brought up here.

In litely, secondary is green -- the "primary" accent color. In darkly, secondary is a a very dark gray. So using the same class -- btn-outline-secondary -- results in unpredictably different outcomes. We're using secondary to mean different things in different themes.

I think maybe the best solution for now (other than revisiting the way we use secondary) would be to do this:

_variables.darkly.scss:

$secondary: $gray-600;

It might also be worth considering adding the text-dark class to the non-active radio toggles so that the text has a higher contrast than the borders. This would make the litely ones have dark gray text instead of green text, however -- another way the inconsistent theme color naming would nail us.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also just noticing that having $input-border-color: $body-bg in darkly is kind of an odd choice:

Screenshot 2023-07-04 at 12 58 40 PM

@@ -55,5 +56,3 @@ hr.my-3 {
display: none;
}
}

@import "../../../../node_modules/bootstrap/scss/bootstrap";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's necessary that this come at the end, since $container-max-widths is being written here.

Also, I believe as of Bootstrap 5, $container-max-widths (and other maps) need to be spelled out in full; they're not longer merged with the defaults.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's necessary that this come at the end, since $container-max-widths is being written here.

If I put the bootstrap import at the end, wouldn't that override the variables set higher up in the file?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, Bootstrap uses the Sass !default flag so as not to overwrite any variables you've set.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, Bootstrap uses Sass's !default flag so as not to overwrite any vars you may have set yourself

@SleeplessOne1917 SleeplessOne1917 enabled auto-merge (squash) July 7, 2023 18:24
@SleeplessOne1917 SleeplessOne1917 merged commit 45333d8 into main Jul 7, 2023
dessalines pushed a commit that referenced this pull request Jul 10, 2023
* Make outline buttons have better contrast in dark themes

* Change secondary color for darkly themes

* Put compact styles back to how they were before

* Forgot to build themes
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.

4 participants