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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

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

Conversation

bew
Copy link
Contributor

@bew bew commented May 7, 2024

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
@@ -126,7 +126,7 @@ impl UiStyles {

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

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

PThorpe92
PThorpe92 previously approved these changes May 22, 2024
@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 from bc5d9b4 to aac96d1 Compare May 23, 2024 14:04
@cafkafk cafkafk force-pushed the fix/size-coloring-tweaks-without-color-scale branch from aac96d1 to e52c367 Compare May 23, 2024 14:07
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

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?

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

Successfully merging this pull request may close these issues.

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