Skip to content

fix(color-scale): use file size unit custom color when not using color scale #975

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(color-scale): use file size unit custom color when not using color scale #975

wants to merge 1 commit into from

Conversation

@bew
Copy link
Contributor

@bew bew commented May 7, 2024 •
edited
Loading

Fixes #682

After half a year I finally went down the rabbit hole and fixed this bug I reported 🙃

I tested different combination of options and the program seems to work correctly in all cases.

Let me know what you think, are there existing tests for this specific part of the renderer?

See how the file size unit is always using my custom colors when color scale is not applied on the file size:
image

image

image

And when using color scale for the file size:
image

@bew bew requested a review from cafkafk as a code owner May 7, 2024 20:58
bew
bew commented May 7, 2024
View reviewed changes
src/theme/default_theme.rs
impl Size {
pub fn colourful(scale: ColorScaleOptions) -> Self {
if scale.size && scale.mode == ColorScaleMode::Fixed {
if scale.mode == ColorScaleMode::Fixed {
Copy link
Contributor Author

@bew bew May 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

here I really don't understand why we were checking if scale.size was enabled (and if it was normal, why scale.age wasn't checked as well 🤔)

cc: @MartinFillon since you changed that condition last in #702.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well iirc it was causing another bug but idrc

@bew bew mentioned this pull request May 21, 2024
Open
PThorpe92
PThorpe92 previously approved these changes May 22, 2024
View reviewed changes
@PThorpe92
Copy link
Member

This looks good to me 👍 Thanks for the PR!

ping @cafkafk for CO review

@cafkafk cafkafk force-pushed the fix/size-coloring-tweaks-without-color-scale branch 2 times, most recently from aac96d1 to e52c367 Compare May 23, 2024 14:07
cafkafk
cafkafk requested changes May 23, 2024
View reviewed changes
Copy link
Member

@cafkafk cafkafk left a comment

Choose a reason for hiding this comment

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

I removed the two update merge commits and instead rebased onto main.

It's not entirely clear to me whether or not changing if scale.size && scale.mode will introduce a regression for a previously fixed bug, @MartinFillon can you remember what this fixes specifically so we can ensure we don't break something?

@MartinFillon
Copy link
Contributor

Okay so took a deep dive the reason of this was to fix #684

@MartinFillon
Copy link
Contributor

clearly it seems that we have two behaviours conflicting with each other here. We need to decide on which one to keep.

@bew
Copy link
Contributor Author

bew commented May 23, 2024 •
edited
Loading

clearly it seems that we have two behaviours conflicting with each other here. We need to decide on which one to keep.

Hmm I don't see how they are conflicting 🤔
When color-scale is not applied for the file size the colors are used from $EZA_COLORS as you can see in my screenshots in PR' description.
And #684 was also asking for this afaiu. Or am I missing something?

@bew bew mentioned this pull request Jul 13, 2024
Closed
@bew bew changed the title fix(color-scale): file size unit custom color when not using color scale fix(color-scale): use file size unit custom color when not using color scale Oct 15, 2024
@cafkafk cafkafk requested a review from gierens as a code owner January 16, 2025 07:09
gierens
gierens previously approved these changes Mar 27, 2025
View reviewed changes
Copy link
Member

@gierens gierens left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, Thanks for the PR!

@gierens gierens force-pushed the fix/size-coloring-tweaks-without-color-scale branch 2 times, most recently from e52c367 to 4c1a617 Compare March 27, 2025 23:47
@gierens
Copy link
Member

gierens commented Mar 27, 2025

rebased

@bew bew force-pushed the fix/size-coloring-tweaks-without-color-scale branch from 4c1a617 to 89bfd42 Compare September 6, 2025 19:11
@bew
Copy link
Contributor Author

bew commented Sep 6, 2025

Rebased!

Any way this will get merged at some point?
It's easy for me to use this patch thanks to Nix overrides, but it's not so easy for everyone else and I think having the ability to customize the number & the unit independently as documented should just work 🤔

@bew bew mentioned this pull request Oct 13, 2025
Closed
@bew bew force-pushed the fix/size-coloring-tweaks-without-color-scale branch from 89bfd42 to c749375 Compare October 18, 2025 08:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MartinFillon MartinFillon MartinFillon left review comments

@cafkafk cafkafk cafkafk requested changes

@gierens gierens gierens left review comments

@PThorpe92 PThorpe92 PThorpe92 left review comments

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

No open projects
Eza Pull-Requests
Status: 🆕 New

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: Since 0.15.3 EZA_COLORS's size coloring tweaks stopped working

5 participants

@bew @PThorpe92 @MartinFillon @gierens @cafkafk