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

Separate outline color #18109

Merged
merged 7 commits into from
Oct 9, 2023
Merged

Separate outline color #18109

merged 7 commits into from
Oct 9, 2023

Conversation

KTibow
Copy link
Contributor

@KTibow KTibow commented Oct 4, 2023

Proposed change

Sometimes a theme wants to hide dividers (I've wanted to do that many times throughout using HA). However, this makes some buttons and expanders look weird. This PR separates outlines from dividers, into their own color --outline-color (defaulting to the same color as --divider-color, for now).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

N/A

Additional information

  • This PR fixes or closes issue: N/A
  • This PR is related to issue or discussion: N/A
  • Link to documentation pull request: N/A

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

@piitaya
Copy link
Member

piitaya commented Oct 4, 2023

I'm ok to split outline and divider into different variables fo easy theming.

However, it should not change the default theme. All the default color should be the same as before.
With this PR, border color are not consistent anymore across the different page.

CleanShot 2023-10-04 at 10 50 03

CleanShot 2023-10-04 at 10 50 29

@KTibow
Copy link
Contributor Author

KTibow commented Oct 4, 2023

I ended up making it a bit more prominent than it is now instead of using secondary or divider by default - this does mean changing the default theme, but I think it's an improvement:
Before:
image
After:
image

@bramkragten
Copy link
Member

bramkragten commented Oct 5, 2023

Good point, that does look bad. However, while I could make outline color default to the divider color, I think it makes more sense to be a bit more prominent than it is now - this does mean changing the default theme, but I think it's an improvement

Not sure I agree that it is an improvement, can you also share a screenshot in light mode? And of a dashboard with a lot of cards?

@KTibow
Copy link
Contributor Author

KTibow commented Oct 5, 2023

can you also share a screenshot in light mode?

Before:
image
After:
image

And of a dashboard with a lot of cards?

It doesn't affect cards at all, since they already have their own variable.

@bramkragten
Copy link
Member

It doesn't affect cards at all, since they already have their own variable.

ha-card-border-color is derived from divider-color, so should be changed to outline color too, and will change then.

Let's not mix changing the colors (style) with the code changes. Let's do the code changes first, and then you can create a new PR for updating the style if you want.

@KTibow
Copy link
Contributor Author

KTibow commented Oct 9, 2023

outline-color is for places where there is no background or anything else to tell it apart. Cards have a background already, so it makes more sense for them to use the subtler divider-color.
But okay - I'll make outline-color look the same as divider-color so it can be updated in a later PR.

@bramkragten bramkragten merged commit d3fd279 into home-assistant:dev Oct 9, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants