-
-
Notifications
You must be signed in to change notification settings - Fork 158
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 Customizable Border Color Option to Default Layout (#332) #333
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,10 +1,19 @@ | ||||||||||||||||||||||||
import React, { useState } from 'react'; | ||||||||||||||||||||||||
import { Typography, Grid } from '@material-ui/core'; | ||||||||||||||||||||||||
import { Typography, Grid, Tooltip } from '@material-ui/core'; | ||||||||||||||||||||||||
import TemplateCard from '../../organisms/TemplateCard'; | ||||||||||||||||||||||||
import { themes, animations, layouts, fonts, colorValues, quoteTypes } from '../../../config/cardTemplate'; | ||||||||||||||||||||||||
import TextField from '@material-ui/core/TextField'; | ||||||||||||||||||||||||
import Autocomplete from '@material-ui/lab/Autocomplete'; | ||||||||||||||||||||||||
import ContributorsCard from '../../ContributorsCard/ContributorCard' | ||||||||||||||||||||||||
import { makeStyles } from '@material-ui/core/styles'; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const useStyles = makeStyles({ | ||||||||||||||||||||||||
customTooltip: { | ||||||||||||||||||||||||
fontSize: '16px', | ||||||||||||||||||||||||
fontWeight: 'bold' | ||||||||||||||||||||||||
}, | ||||||||||||||||||||||||
}); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const Home = () => { | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const [theme, setTheme] = useState(themes[0]); | ||||||||||||||||||||||||
|
@@ -13,8 +22,11 @@ const Home = () => { | |||||||||||||||||||||||
const [font, setFont] = useState(fonts[0]); | ||||||||||||||||||||||||
const [fontColor, setFontColor] = useState(null); | ||||||||||||||||||||||||
const [bgColor, setBgColor] = useState(null); | ||||||||||||||||||||||||
const [borderColor, setBorderColor] = useState(null); | ||||||||||||||||||||||||
const [quoteType, setQuoteType] = useState("random"); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
const classes = useStyles(); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||
<React.Fragment> | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -112,6 +124,26 @@ const Home = () => { | |||||||||||||||||||||||
/> | ||||||||||||||||||||||||
</Grid> | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
<Grid item xs={12} sm={6} lg={3}> | ||||||||||||||||||||||||
<Tooltip | ||||||||||||||||||||||||
title="This option only works with the default layout." | ||||||||||||||||||||||||
placement="top" | ||||||||||||||||||||||||
arrow | ||||||||||||||||||||||||
classes={{ tooltip: classes.customTooltip }} | ||||||||||||||||||||||||
> | ||||||||||||||||||||||||
<Autocomplete | ||||||||||||||||||||||||
id="border-color" | ||||||||||||||||||||||||
options={colorValues} | ||||||||||||||||||||||||
value={borderColor} | ||||||||||||||||||||||||
style={{ width: 300 }} | ||||||||||||||||||||||||
onChange={(_event, newValue) => { | ||||||||||||||||||||||||
setBorderColor(newValue) | ||||||||||||||||||||||||
}} | ||||||||||||||||||||||||
renderInput={(params) => <TextField {...params} label="Border color" placeholder="Select a color" variant="outlined" />} | ||||||||||||||||||||||||
/> | ||||||||||||||||||||||||
</Tooltip> | ||||||||||||||||||||||||
</Grid> | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
<Grid item xs={12} sm={6} lg={3}> | ||||||||||||||||||||||||
<Autocomplete | ||||||||||||||||||||||||
id="quote-type" | ||||||||||||||||||||||||
|
@@ -130,7 +162,7 @@ const Home = () => { | |||||||||||||||||||||||
|
||||||||||||||||||||||||
<Grid container spacing={4}> | ||||||||||||||||||||||||
<Grid item xs={12} style={{ marginTop: '20px' }}> | ||||||||||||||||||||||||
<TemplateCard theme={theme} animation={animation} layout={layout} font={font} fontColor={fontColor} bgColor={bgColor} quoteType={quoteType} /> | ||||||||||||||||||||||||
<TemplateCard theme={theme} animation={animation} layout={layout} font={font} fontColor={fontColor} bgColor={bgColor} borderColor={borderColor} quoteType={quoteType} /> | ||||||||||||||||||||||||
</Grid> | ||||||||||||||||||||||||
<Grid item xs={12}> | ||||||||||||||||||||||||
<Typography align="center">Other layouts</Typography> | ||||||||||||||||||||||||
|
@@ -139,7 +171,7 @@ const Home = () => { | |||||||||||||||||||||||
layouts.filter((item) => item !== layout).map((restLayout) => { | ||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||
<Grid key={restLayout} item xs={12} sm={12} md={6}> | ||||||||||||||||||||||||
<TemplateCard theme={theme} animation={animation} layout={restLayout} font={font} fontColor={fontColor} bgColor={bgColor} quoteType={quoteType} /> | ||||||||||||||||||||||||
<TemplateCard theme={theme} animation={animation} layout={restLayout} font={font} fontColor={fontColor} bgColor={bgColor} borderColor={borderColor} quoteType={quoteType} /> | ||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Optimize borderColor prop passing The borderColor prop is being passed to all layouts, but it only works with the default layout. Consider conditionally passing the prop. -<TemplateCard theme={theme} animation={animation} layout={restLayout} font={font} fontColor={fontColor} bgColor={bgColor} borderColor={borderColor} quoteType={quoteType} />
+<TemplateCard
+ theme={theme}
+ animation={animation}
+ layout={restLayout}
+ font={font}
+ fontColor={fontColor}
+ bgColor={bgColor}
+ {...(restLayout === 'default' ? { borderColor } : {})}
+ quoteType={quoteType}
+/> 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
</Grid> | ||||||||||||||||||||||||
) | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,6 +18,8 @@ const quoteController = async (req, res, next) => { | |||||||||||||||||||||||
theme.bg_color = bgColor; | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
let borderColor = req.query.borderColor || 'rgba(0, 0, 0, 0.2)'; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
Comment on lines
+21
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||
let animation = animations[req.query.animation] ? animations[req.query.animation] | ||||||||||||||||||||||||
: animations["default"]; | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
@@ -39,10 +41,10 @@ const quoteController = async (req, res, next) => { | |||||||||||||||||||||||
quotesUrl, | ||||||||||||||||||||||||
quoteCategory, | ||||||||||||||||||||||||
font, | ||||||||||||||||||||||||
quoteType | ||||||||||||||||||||||||
quoteType, | ||||||||||||||||||||||||
borderColor | ||||||||||||||||||||||||
} | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
let svgResponse = await quoteService.getQuote(quoteObject); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
res.setHeader("Content-Type", "image/svg+xml"); | ||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation for the borderColor parameter. The 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');
+ }
|
||
let apiResponse; | ||
let { customQuotesUrl, isValidUrl } = await getValidUrl(quotesUrl); | ||
let isCustomQuote = false; | ||
|
@@ -49,6 +49,7 @@ const getQuote = async (quoteObj) => { | |
template.setData(isCustomQuote ? apiResponse : apiResponse[Math.floor(getQuoteIndex(apiResponse.length, quoteType))]); | ||
template.setFont(font); | ||
template.setAnimation(animation); | ||
template.setBorderColor(borderColor); | ||
template.setLayout(layout); | ||
|
||
let svg = cardTemplate.generateTemplate(template); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add validation and fallback for border color. While the implementation adds the requested border color customization, there are some concerns that should be addressed:
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)';
}; |
||
border-radius: 5px; | ||
${template.animation.animation}; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,11 @@ class Template { | |
this.setStructure(layout.structure); | ||
this.calculateHeight(this.quote.length); | ||
} | ||
|
||
setBorderColor(borderColor) { | ||
this.borderColor = borderColor; | ||
} | ||
Comment on lines
+26
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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;
}
|
||
|
||
setStyle(style) { | ||
this.css = style(this); | ||
} | ||
|
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.
🛠️ Refactor suggestion
Enhance the feature description with supported color formats.
The documentation should specify:
📝 Committable suggestion