Add Customizable Border Color Option to Default Layout (#332) #333
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@marceloams is attempting to deploy a commit to the shravan20's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes in this pull request introduce a customizable border color feature for quote templates. The Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
src/models/Template.js (1)
26-28: Add JSDoc documentation for consistencyConsider adding JSDoc documentation to maintain consistency with the codebase style.
Add documentation like this:
+ /** + * Sets the border color for the template + * @param {string} borderColor - CSS color value (hex, rgb, rgba, or named color) + * @throws {Error} If the color format is invalid + */ setBorderColor(borderColor) {src/api/controllers/quotesController.js (1)
21-22: Consider architectural improvements.While the implementation works, consider these enhancements for better maintainability:
- Move the default border color to a constants file along with other default values
- Add JSDoc comments to document the new borderColor parameter
Example implementation:
+/** + * @typedef {Object} QuoteParams + * @property {string} [borderColor] - Custom border color in valid CSS format (hex, rgb, rgba, or named color) + */ +// In constants.js +const DEFAULT_BORDER_COLOR = 'rgba(0, 0, 0, 0.2)'; - let borderColor = req.query.borderColor || 'rgba(0, 0, 0, 0.2)'; + let borderColor = req.query.borderColor || DEFAULT_BORDER_COLOR;src/api/services/quotesService.js (2)
Line range hint
56-58: Enhance error handling with specific error types.The current error handling simply re-throws the error without providing specific context about what went wrong.
Consider implementing more specific error handling:
} catch (error) { - throw error; + if (error.message === 'Invalid border color provided') { + throw new Error(`Invalid border color: ${borderColor}. Please provide a valid CSS color value.`); + } + throw new Error(`Failed to generate quote template: ${error.message}`); }
Line range hint
13-17: Add JSDoc documentation for the getQuote function.The function parameters and their types should be documented, especially with the new
borderColorparameter.Add documentation above the function:
+/** + * Generates a quote template with customization options + * @param {Object} quoteObj - The quote configuration object + * @param {string} [quoteObj.borderColor] - CSS color value for the template border + * @param {string} [quoteObj.theme] - Template theme + * @param {string} [quoteObj.animation] - Animation type + * @param {string} [quoteObj.layout] - Template layout + * @param {string} [quoteObj.quotesUrl] - Custom quotes URL + * @param {string} [quoteObj.quoteCategory] - Quote category + * @param {string} [quoteObj.font] - Font family + * @param {string} [quoteObj.quoteType] - Type of quote + * @returns {Promise<string>} The generated SVG template + * @throws {Error} When border color is invalid or template generation fails + */README.md (2)
245-246: Fix typo in parameter description.There's a typo in the word "parameter".
-Use `?borderColor=COLOR` paramater as shown below +Use `?borderColor=COLOR` parameter as shown below
247-252: Add more comprehensive examples.The example could be enhanced by showing multiple color format options.
```md  +
+</blockquote></details> <details> <summary>src/layouts/layout.js (1)</summary><blockquote> `14-14`: **Consider extending border customization across all layouts.** The current implementation only allows border color customization in the default layout. Consider: 1. Extending this feature to other layouts for consistency 2. Creating a comprehensive border customization system that could include: - Border width - Border style - Border radius This would provide a more consistent and scalable approach across all layouts. Would you like me to propose a comprehensive border customization implementation that works across all layouts? </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used: CodeRabbit UI** **Review profile: CHILL** <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between f3f290228d0dffef80ef5de4f776dd5ddeb8a4ae and 8ed21011ec7f6d04b3b56cddbbc69d57f5149d1f. </details> <details> <summary>📒 Files selected for processing (5)</summary> * `README.md` (1 hunks) * `src/api/controllers/quotesController.js` (2 hunks) * `src/api/services/quotesService.js` (2 hunks) * `src/layouts/layout.js` (1 hunks) * `src/models/Template.js` (1 hunks) </details> <details> <summary>🔇 Additional comments (5)</summary> <details> <summary>src/api/controllers/quotesController.js (2)</summary> `44-45`: **LGTM!** The borderColor property is correctly added to the quoteObject, maintaining consistent formatting with other properties. --- Line range hint `21-45`: **Verify template integration.** Let's ensure the borderColor parameter is properly integrated with the template system. <details> <summary>✅ Verification successful</summary> **Border color parameter is properly integrated with the template system** The verification shows that: - The border color is correctly applied in the layout template (`layout.js`) using template literals - The Template model (`Template.js`) has the proper setter method `setBorderColor` - The quotes service correctly sets the border color on the template before generation - The implementation forms a complete chain from controller to final rendering </details> <details> <summary>🏁 Scripts executed</summary> The following scripts were executed for the analysis: Script: ```shell #!/bin/bash # Description: Verify the borderColor parameter integration with templates # Check if the template system handles the borderColor parameter echo "Checking template integration..." rg -l "borderColor" "src/layouts/" # Verify error handling in the template system echo "Checking error handling..." rg -l "setBorderColor" "src/api/services/"Length of output: 267
Script:
#!/bin/bash # Let's examine how borderColor is used in the layout and service files echo "Checking layout.js implementation:" rg -A 5 "borderColor" "src/layouts/layout.js" echo "\nChecking quotesService.js implementation:" rg -A 5 "setBorderColor" "src/api/services/quotesService.js" # Also check if there are any template files that might need the parameter echo "\nChecking template files:" fd "template" --type f --exec rg -l "border"Length of output: 817
Script:
#!/bin/bash # Let's check the Template model implementation echo "Checking Template.js implementation:" rg -A 5 "setBorderColor|borderColor" "src/models/Template.js" # Let's also verify if there are any test files covering this functionality echo "\nChecking test coverage:" fd "test|spec" --type f --exec rg -l "borderColor"Length of output: 381
src/api/services/quotesService.js (1)
52-52: Verify Template class implementation.
The code calls setBorderColor on the template instance, but we need to verify its implementation in the Template class.
✅ Verification successful
setBorderColor method is properly implemented and used correctly
The Template class in src/models/Template.js has a proper implementation of the setBorderColor method that stores the border color in the instance property. The usage in quotesService.js is consistent with this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Template class implementation and usage
# Test 1: Check if setBorderColor method is properly implemented
ast-grep --pattern 'class Template {
$$$
setBorderColor($_) {
$$$
}
$$$
}'
# Test 2: Check for any existing usage of setBorderColor
rg "setBorderColor" -A 2
Length of output: 2665
README.md (1)
239-254: Documentation aligns well with the PR objectives.
The new border color feature is well-documented and matches the requirements from issue #332. The section maintains consistency with the rest of the README structure and provides clear usage instructions.
src/layouts/layout.js (1)
14-14: Verify template.borderColor implementation.
Let's verify the complete implementation of the border color feature across the codebase.
✅ Verification successful
Let's gather more specific information about the implementation by checking the actual code where borderColor is used.
Border color implementation is properly integrated across the codebase
The implementation of template.borderColor is correctly integrated throughout the application:
- Defined in Template model with proper setter method
- Used in layout.js for border styling
- Handled in API layer with a default value of 'rgba(0, 0, 0, 0.2)'
- Properly passed through the service layer to template generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of template.borderColor
# Test 1: Check for borderColor property definition
echo "Checking for borderColor property definition..."
rg -l "borderColor|border.*color" --type js
# Test 2: Check for related test files
echo -e "\nChecking for related test files..."
fd -e test.js -e spec.js
# Test 3: Check for validation implementation
echo -e "\nChecking for color validation..."
rg -l "validateColor|sanitizeColor|isValidColor" --type js
# Test 4: Check for documentation
echo -e "\nChecking for documentation..."
fd README.md | xargs rg -l "borderColor"
Length of output: 724
Script:
#!/bin/bash
# Check Template model definition
echo "=== Template.js model definition ==="
rg -A 5 -B 5 "borderColor" src/models/Template.js
# Check layout implementation
echo -e "\n=== Layout implementation ==="
rg -A 5 -B 5 "borderColor" src/layouts/layout.js
# Check frontend usage
echo -e "\n=== Frontend usage ==="
rg -A 5 -B 5 "borderColor" frontend/src/util/layouts/index.js
# Check API implementation
echo -e "\n=== API implementation ==="
rg -A 5 -B 5 "borderColor" src/api/services/quotesService.js
rg -A 5 -B 5 "borderColor" src/api/controllers/quotesController.js
Length of output: 2599
| setBorderColor(borderColor) { | ||
| this.borderColor = borderColor; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for borderColor parameter
The current implementation directly assigns the input without validation, which could lead to security issues or rendering problems. Consider adding validation to ensure only valid CSS color values are accepted.
Here's a suggested implementation:
setBorderColor(borderColor) {
+ // Validate color format (hex, rgb, rgba, named colors)
+ const isValidColor = (color) => {
+ const style = new Option().style;
+ style.color = color;
+ return style.color !== '';
+ };
+
+ if (!borderColor) {
+ return;
+ }
+
+ if (!isValidColor(borderColor)) {
+ throw new Error('Invalid border color format');
+ }
+
this.borderColor = borderColor;
}Committable suggestion skipped: line range outside the PR's diff.
| let borderColor = req.query.borderColor || 'rgba(0, 0, 0, 0.2)'; | ||
|
|
There was a problem hiding this comment.
Add input validation for borderColor parameter.
The borderColor parameter is used directly from user input without validation. This could potentially lead to XSS attacks or invalid CSS values.
Consider adding color validation before using the input:
- let borderColor = req.query.borderColor || 'rgba(0, 0, 0, 0.2)';
+ let borderColor = 'rgba(0, 0, 0, 0.2)';
+ if (req.query.borderColor) {
+ const isValidColor = /^(#[0-9A-Fa-f]{6}|rgb\(\d{1,3},\s*\d{1,3},\s*\d{1,3}\)|rgba\(\d{1,3},\s*\d{1,3},\s*\d{1,3},\s*([01]|0?\.\d+)\)|[a-zA-Z]+)$/
+ .test(req.query.borderColor);
+ if (isValidColor) {
+ borderColor = req.query.borderColor;
+ } else {
+ console.warn(`Invalid border color provided: ${req.query.borderColor}`);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let borderColor = req.query.borderColor || 'rgba(0, 0, 0, 0.2)'; | |
| let borderColor = 'rgba(0, 0, 0, 0.2)'; | |
| if (req.query.borderColor) { | |
| const isValidColor = /^(#[0-9A-Fa-f]{6}|rgb\(\d{1,3},\s*\d{1,3},\s*\d{1,3}\)|rgba\(\d{1,3},\s*\d{1,3},\s*\d{1,3},\s*([01]|0?\.\d+)\)|[a-zA-Z]+)$/ | |
| .test(req.query.borderColor); | |
| if (isValidColor) { | |
| borderColor = req.query.borderColor; | |
| } else { | |
| console.warn(`Invalid border color provided: ${req.query.borderColor}`); | |
| } | |
| } |
| @@ -15,7 +15,7 @@ getQuoteIndex = (apiResponseLength, quoteType) => { | |||
| const getQuote = async (quoteObj) => { | |||
|
|
|||
| try { | |||
| let { theme, animation, layout, quotesUrl, quoteCategory, font, quoteType } = quoteObj; | |||
| let { theme, animation, layout, quotesUrl, quoteCategory, font, quoteType, borderColor } = quoteObj; | |||
There was a problem hiding this comment.
Add validation for the borderColor parameter.
The borderColor parameter is used directly without validation. This could lead to XSS vulnerabilities or visual inconsistencies if invalid color values are provided.
Consider adding color validation before using the parameter:
- let { theme, animation, layout, quotesUrl, quoteCategory, font, quoteType, borderColor } = quoteObj;
+ let { theme, animation, layout, quotesUrl, quoteCategory, font, quoteType, borderColor = '#000000' } = quoteObj;
+ // Add this validation function to utils
+ const isValidColor = (color) => {
+ const s = new Option().style;
+ s.color = color;
+ return s.color !== '';
+ };
+ if (borderColor && !isValidColor(borderColor)) {
+ throw new Error('Invalid border color provided');
+ }Committable suggestion skipped: line range outside the PR's diff.
| You can customize the border color of your templates. Please note that this feature is available only with the Default layout. | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the feature description with supported color formats.
The documentation should specify:
- Which color formats are supported (hex codes, named colors, rgb values?)
- What is the default border color when the parameter is not provided
-You can customize the border color of your templates. Please note that this feature is available only with the Default layout.
+You can customize the border color of your templates using CSS color names (e.g., 'red', 'blue') or hex codes (e.g., '#FF0000'). The default border color is gray. Please note that this feature is available only with the Default layout.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| You can customize the border color of your templates. Please note that this feature is available only with the Default layout. | |
| You can customize the border color of your templates using CSS color names (e.g., 'red', 'blue') or hex codes (e.g., '#FF0000'). The default border color is gray. Please note that this feature is available only with the Default layout. |
| @@ -11,7 +11,7 @@ const layouts = { | |||
| padding: 40px 20px; | |||
| min-width: 600px; | |||
| background: ${template.theme.bg_color}; | |||
| border: 1px solid rgba(0, 0, 0, 0.2); | |||
| border: 1px solid ${template.borderColor}; | |||
There was a problem hiding this comment.
Add validation and fallback for border color.
While the implementation adds the requested border color customization, there are some concerns that should be addressed:
- No fallback color is provided when
template.borderColoris undefined - Missing validation could allow invalid CSS values
- Direct use of user input without sanitization poses a potential CSS injection risk
Consider applying this safer implementation:
- border: 1px solid ${template.borderColor};
+ border: 1px solid ${template.borderColor ? sanitizeColor(template.borderColor) : 'rgba(0, 0, 0, 0.1)'};Add this helper function at the top of the file:
const sanitizeColor = (color) => {
// Allow named colors, hex, rgb, rgba, hsl, hsla
const safeColorPattern = /^(#([0-9A-Fa-f]{3,8})|rgb[a]?\([\d\s,%.]+\)|hsl[a]?\([\d\s,%]+\)|[a-z-]+)$/;
return safeColorPattern.test(color) ? color : 'rgba(0, 0, 0, 0.1)';
};
marceloams
force-pushed
the
332-add-customizable-border-color-option-to-default-layout
branch
from
8ed2101
to
857accd
Compare
marceloams
force-pushed
the
332-add-customizable-border-color-option-to-default-layout
branch
from
857accd
to
7d4b15f
Compare
Overview
This PR introduces an option to customize the border color in the default layout, enhancing the visual flexibility of the templates.
Changes
Closes #332.
Summary by CodeRabbit
?borderColor=COLORparameter.README.mdto include new border color customization feature and clarified instructions for adding quotes to categories.