-
Notifications
You must be signed in to change notification settings - Fork 2
Issue 54: make colorblind friendly #60
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
Conversation
WalkthroughThis pull request introduces shape aesthetics for models in plotting functions, adds a new Changes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
R/fig_overall_scores.R (1)
603-637: Missingscale_shape_manualin absolute scores block.Whilst the shape aesthetic is mapped to
modelat line 621 and the shape legend is configured in guides, thescale_shape_manualcall is missing. This means ggplot2 will use default shapes rather than the custommodel_shapesdefined inplot_components(), defeating the accessibility purpose of this PR.In contrast, the relative skill block (lines 560-596) correctly includes
scale_shape_manualat line 584.🔎 Proposed fix
) ) + scale_color_manual(values = plot_components_list$model_colors) + + scale_shape_manual(values = plot_components_list$model_shapes) + geom_vline(xintercept = 0, linetype = "dashed", color = "gray50") + get_plot_theme() +
🧹 Nitpick comments (1)
R/fig_overall_scores.R (1)
500-515: Clarify the interaction betweenremove_legendandadd_shapeparameters.The current logic allows
add_shape = TRUEto overrideremove_legend = TRUEfor the shape legend specifically. This creates an asymmetric API where:
- If
remove_legend = TRUEandadd_shape = TRUE: shape legend shows, but colour/fill legends are hidden- If
remove_legend = TRUEandadd_shape = FALSE: all legends are hiddenWhilst this may be intentional, consider documenting this behaviour explicitly in the function documentation to prevent confusion.
💡 Suggested documentation enhancement
#' @param remove_legend Boolean indicating whether to keep legend, default #' is TRUE. #' @param add_shape Boolean indicating whether to add the shape legend, -#' default is FALSE. +#' default is FALSE. Note: when TRUE, this overrides remove_legend for +#' the shape aesthetic only.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
R/fig_overall_scores.RR/plotting_style.Rtargets/fig_overall_targets.R
🔇 Additional comments (5)
R/plotting_style.R (1)
60-67: Excellent addition of shape aesthetics for accessibility.The shape mappings are well-chosen and use standard ggplot2 shape codes that will provide clear visual distinction between models for colorblind users.
targets/fig_overall_targets.R (1)
105-105: Legend visibility controls are well-coordinated.The selective legend removal for CA plots and by-location plots aligns well with the multi-panel figure layout strategy where legends are collected at the top level.
Also applies to: 113-114, 122-123, 222-223
R/fig_overall_scores.R (3)
74-111: Shape aesthetics correctly integrated for accessibility.The implementation properly adds shape mapping alongside colour, includes the appropriate
scale_shape_manual, and coordinates legend guides for both aesthetics.
197-227: Shape aesthetics consistently applied across both plot types.Both the relative skill and absolute score paths correctly implement shape mapping with proper scales and legend configurations.
Also applies to: 247-296
674-675: Consistent bar chart styling applied.The use of fixed black fill for sequence count bar charts provides clear, simple visualisation without redundant legend items.
Also applies to: 700-701, 728-729
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.