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

Add colors to summary #463

Merged
merged 3 commits into from
Nov 12, 2021
Merged

Conversation

limdingwen
Copy link
Contributor

@limdingwen limdingwen commented Nov 10, 2021

Uses the existing intervalColor function. Includes documentation consistent with CmdTags and CmdChart.

Screenshot 2021-11-11 at 1 40 19 AM

Closes #459.

Signed-off-by: Lim Ding Wen <limdingwen@gmail.com>
@limdingwen
Copy link
Contributor Author

limdingwen commented Nov 10, 2021

Hang on, realised this doesn't correctly handle cases where the tag has no color.

Documenting down differences in color handling logic:

  • Tags: Only one allowed, no color if empty
  • Summary: Multiple allowed, no color if empty
  • Chart: Multiple allowed, show palette color if empty

Because of this, I'll need to adapt the logic from intervalColor slightly to accommodate Summary.

Edit: Implementation screenshot

Screenshot 2021-11-11 at 2 34 20 AM

@limdingwen
Copy link
Contributor Author

limdingwen commented Nov 10, 2021

I'm unable to test if the code works with color = off, due to bug #464, but from the code it seems that CmdTags does not check that flag when displaying colors?

Edit: On second look, it seems like color is intended to only affect theme colors, not tag colors.

Signed-off-by: Lim Ding Wen <limdingwen@gmail.com>
@limdingwen
Copy link
Contributor Author

limdingwen commented Nov 10, 2021

The PR is ready to be reviewed! 😄

Not sure what's with the GH tests, it worked on the previous commit :P This commit literally only added 2 whitespaces to fix the formatting

Signed-off-by: Lim Ding Wen <limdingwen@gmail.com>
Copy link
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@lauft
Copy link
Member

lauft commented Nov 12, 2021

Not sure what's with the GH tests, it worked on the previous commit :P This commit literally only added 2 whitespaces to fix the formatting

The error in the previous run were due to another issue (already fixed) connected with the execution time. See 👉🏻 #466

@lauft lauft merged commit 82b7e55 into GothenburgBitFactory:dev Nov 12, 2021
@limdingwen limdingwen deleted the summary-colors branch November 13, 2021 01:22
@lauft lauft added this to the 1.5.0 milestone Sep 27, 2022
@lauft lauft added the enhancement New feature or request label Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tag coloring to summary
2 participants