feat: add for-the-badge style
#73
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe changes in this pull request introduce a new style option called Changes
Assessment against linked issues
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
gstraccini
bot
added
enhancement
![gstraccini[bot]](https://avatars.githubusercontent.com/in/480132?s=60&v=4){kind=small}
gstraccini
bot
added
the
🚦 awaiting triage
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app.py (2)
90-100: Consider adding documentation for the style's visual characteristicsThe
for-the-badgestyle properties are well-configured to match the expected visual appearance. Consider adding a comment block above the style definition to document its distinctive visual characteristics for future maintainers.}, + # The "for-the-badge" style features: + # - Bold, centered text with increased letter spacing + # - Sharp edges (no border radius) + # - Solid colors (no gradient) + # - Taller height for better visibility "for-the-badge": {
166-180: Improve maintainability of text positioning calculationsWhile the positioning logic is functional, consider these improvements:
- The letter spacing fallback value (1) differs from the default style's value (0)
- The magic numbers for positioning (5, 14, 18) should be documented or defined as constants
+ # Constants for text positioning + TEXT_PADDING_LEFT = 5 + TEXT_BASELINE_LEFT = 14 + TEXT_BASELINE_CENTER = 18 if style.get("title_anchor") == "left": - clean_req_params["title_pos_x"] = 5 - clean_req_params["title_pos_y"] = 14 + clean_req_params["title_pos_x"] = TEXT_PADDING_LEFT + clean_req_params["title_pos_y"] = TEXT_BASELINE_LEFT elif style.get("title_anchor") == "middle": clean_req_params["title_pos_x"] = (clean_req_params.get("title_width", 0) / 2) - clean_req_params["title_pos_y"] = 18 + clean_req_params["title_pos_y"] = TEXT_BASELINE_CENTERAlso, ensure the letter spacing fallback matches the default style:
- clean_req_params["title_width"] = style.get("title_width") + 10 + (6 * len(clean_req_params["title"])) + (len(clean_req_params["title"]) * style.get("letter_spacing", 1)) + clean_req_params["title_width"] = style.get("title_width") + 10 + (6 * len(clean_req_params["title"])) + (len(clean_req_params["title"]) * style.get("letter_spacing", 0))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
templates/progress.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
README.md(1 hunks)app.py(3 hunks)
🔇 Additional comments (2)
app.py (2)
65-71: LGTM: Well-structured style properties
The new style properties provide a solid foundation for customizable SVG styling with sensible defaults.
161-161: LGTM: Clean style merging implementation
The dictionary unpacking approach ensures proper override behavior where style-specific values take precedence over defaults.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app.py (2)
65-71: LGTM! Consider documenting the new style properties.The new style properties are well-chosen and provide good control over typography and layout. The default values are reasonable.
Consider adding docstring documentation for these new properties to help users understand their purpose and valid values.
166-173: Consider adding maximum title width constraint.The title width calculation looks good, but very long titles could cause layout issues. Consider adding a maximum width constraint or text truncation.
if "title" in clean_req_params and clean_req_params["title"] != "": + max_title_width = 200 # Adjust as needed title_width = ( style.get("title_width") + 10 + (6 * len(clean_req_params["title"])) + (len(clean_req_params["title"]) * style.get("letter_spacing", 1)) ) + clean_req_params["title_width"] = min(title_width, max_title_width) - clean_req_params["title_width"] = title_width else: clean_req_params["title_width"] = 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
README.md(1 hunks)app.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (3)
app.py (3)
161-161: LGTM! Style merging logic is robust.
The style merging approach correctly preserves defaults while allowing style-specific overrides.
90-100: Verify the style rendering with various title lengths.
The style properties look good and align with the "For The Badge" style requirements. However, the fixed title_width of 10 combined with title_anchor: "middle" needs verification with different title lengths.
Let's verify the rendering with different title lengths:
✅ Verification successful
The previous attempt to test the endpoint directly wasn't successful since we're in a sandbox environment. Instead, let's analyze the codebase to understand how the title width is handled, which will help us determine if the fixed width of 10 could be problematic.
Fixed title width is not an issue - dynamic calculation is implemented
The codebase analysis shows that despite the initial title_width: 10 in the style template, there's a dynamic width calculation that properly handles varying title lengths. The actual width is calculated using the formula:
title_width = style.title_width + 10 + (6 * len(title)) + (len(title) * letter_spacing)This means:
- Base width: 10 (from template)
- Padding: +10
- Character width: 6 units per character
- Letter spacing: 2 units per character (from template)
The title_anchor: "middle" positioning is then correctly calculated using this dynamic width, so there won't be any rendering issues with different title lengths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Test the SVG generation with various title lengths
# Note: Using curl to test the endpoint directly
# Test cases with different title lengths
titles=("" "A" "Test" "Very Long Title")
for title in "${titles[@]}"; do
echo "Testing title: $title"
curl -s "http://localhost:5000/50?style=for-the-badge&title=$title" | grep -A 1 "<text"
done
Length of output: 722
Script:
#!/bin/bash
# Let's analyze the code to understand how title width is calculated and handled
# First, let's find the SVG generation logic
rg -l "def.*svg" --type py
# Then, let's look for title width calculation
rg "title_width|width.*calc" --type py -A 3 -B 3
# Also check for any dynamic width adjustments
ast-grep --pattern 'width = $_'
Length of output: 1642
180-186: Verify title positioning with edge cases.
The title positioning logic looks good but should be tested with edge cases:
- Very long titles
- Titles with special characters
- RTL text
Let's verify the positioning with edge cases:
guibranco
removed
the
🚦 awaiting triage
|
Hi @ztest95 👋, Thank you so much for your pull request! 🙌 I appreciate the time and effort you put into this contribution. Thanks again for your valuable contribution! 🚀 |
|
@gstraccini review |
|
Reviewing this pull request! 👀 Commits included: |
Closes #29
📑 Description
for-the-badgestyle.letter_spacing,font_size,height,title_anchor,gradientwhich are needed to modify the svg for this style. these are only defined, currently, within thefor-the-badgestyle, and are not user customizable✅ Checks
☢️ Does this introduce a breaking change?
ℹ Additional Information
(on the above image)
for-the-badgestylesquarestylei wasn't sure if the
for-the-badgestyle should force capitalized titlesSummary by CodeRabbit
Summary by CodeRabbit
New Features
for-the-badge.Documentation
for-the-badgestyle with an example preview and URL.