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 Customizable Border Color Option to Default Layout (#332) #333

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,20 @@ You can also provide a category to fetch the list of quotes based on certain cat

---

- ### Border Color

You can customize the border color of your templates. Please note that this feature is available only with the Default layout.

Comment on lines +248 to +249
Copy link

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:

  • 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.

Suggested change
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.

Use `?borderColor=COLOR` paramater as shown below

```md
![Quote](https://github-readme-quotes-bay.vercel.app/quote?borderColor=green)
```

![Quote](https://github-readme-quotes-bay.vercel.app/quote?borderColor=green)

---

## Swagger Docs

To view Swagger docs, run `npm start` and open ![localhost:3002/api-docs](localhost:3002/api-docs).
Expand Down
38 changes: 35 additions & 3 deletions frontend/src/components/Pages/Home/index.js
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]);
Expand All @@ -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>

Expand Down Expand Up @@ -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"
Expand All @@ -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>
Expand All @@ -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} />
Copy link

Choose a reason for hiding this comment

The 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

‼️ 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.

Suggested change
<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}
/>

</Grid>
)
})
Expand Down
7 changes: 6 additions & 1 deletion frontend/src/components/organisms/TemplateCard/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,14 @@ const TemplateCard = (props) => {
theme.bg_color = props.bgColor;
}

const isLayoutDefault = props.layout === 'default';
const borderColor = isLayoutDefault && props.borderColor ? props.borderColor : 'rgba(0, 0, 0, 0.2)';

template.setTheme(theme);
template.setData(data);
template.setFont(mainFonts[props.font]);
template.setAnimation(mainAnimations[props.animation]);
template.setBorderColor(borderColor);
template.setLayout(mainLayouts[props.layout]);
const file = new Blob([getTemplate(template)], { type: "image/svg+xml" });
const url = URL.createObjectURL(file);
Expand Down Expand Up @@ -72,7 +76,8 @@ const TemplateCard = (props) => {
font: props.font,
quoteType: props.quoteType,
...(props.bgColor && { bgColor: props.bgColor }),
...(props.fontColor && { fontColor: props.fontColor })
...(props.fontColor && { fontColor: props.fontColor }),
...(isLayoutDefault && props.borderColor && { borderColor }),
});
const quoteUrl = `${originUrl}/quote?${params.toString()}`;

Expand Down
2 changes: 1 addition & 1 deletion frontend/src/util/layouts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const layouts = {
padding: 40px 20px;
min-width: 600px;
background-color: ${template.theme.bg_color};
border: 1px solid rgba(0, 0, 0, 0.2);
border: 1px solid ${template.borderColor};
border-radius: 5px;
${template.animation.animation};
}
Expand Down
5 changes: 5 additions & 0 deletions frontend/src/util/template/Template.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ class Template {
this.setStructure(layout.structure);
this.calculateHeight(this.quote.length);
}

setBorderColor(borderColor) {
this.borderColor = borderColor;
}

setStyle(style) {
this.css = style(this);
}
Expand Down
6 changes: 4 additions & 2 deletions src/api/controllers/quotesController.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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}`);
}
}

let animation = animations[req.query.animation] ? animations[req.query.animation]
: animations["default"];

Expand All @@ -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");
Expand Down
3 changes: 2 additions & 1 deletion src/api/services/quotesService.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

let apiResponse;
let { customQuotesUrl, isValidUrl } = await getValidUrl(quotesUrl);
let isCustomQuote = false;
Expand Down Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/layouts/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add validation and fallback for border color.

While the implementation adds the requested border color customization, there are some concerns that should be addressed:

  1. No fallback color is provided when template.borderColor is undefined
  2. Missing validation could allow invalid CSS values
  3. 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)';
};

border-radius: 5px;
${template.animation.animation};
}
Expand Down
5 changes: 5 additions & 0 deletions src/models/Template.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

The 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;
  }

Committable suggestion skipped: line range outside the PR's diff.


setStyle(style) {
this.css = style(this);
}
Expand Down