-
Notifications
You must be signed in to change notification settings - Fork 3
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
Added color to the images based on the learning path #391
Conversation
Coder-Srinivas
commented
Aug 5, 2024
- The color of the study images is now based on the color of the learning path.
- Added a placeholder image instead of using the AWS bucket.
- Removed the eye dropper from color input
- The secondary color of the images is replaced with the color of the learning path
import { getImageUrl, getAltText } from './card-images' | ||
import { colors } from '@theme'; | ||
|
||
const SVGManipulator: React.FC<{src: string, fillColor: string, altText: string}> = ({ src, fillColor, altText }) => { |
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.
I am not an expert with SVGs so im curious about this approach, it looks like we are modifying the fill
property in the SVG and then replacing the image?
would this be possible/simpler with CSS? something like this? not sure if it work, but would simplify this a bit: https://stackoverflow.com/questions/9529300/can-i-change-the-fill-color-of-an-svg-path-with-css
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.
I tried that approach initially and found that since we are using an img tag and using the svg as a src, it would not work. For the above approach to work, the svg file should be rendered directly instead of using an img tag, since the images are in the public folder, I could not import them, to use them directly.
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.
Another problem I noticed with that approach is that on setting the fill values on the svg container, it automatically applies to all the paths, unless a fill property is already present. So we have to manually edit all the paths which don't have a fill value and set it to none, and remove the fill value for the paths which have the secondary color. This way by setting the fill value once, it is applied to all the paths which don't have a fill value (secondary color). The only problem is, the svg files have to be edited manually and should be placed inside the src folder
|
||
return ( | ||
<SVGManipulator | ||
src={getImageUrl(study.imageId)} |
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.
More a question of where getImageUrl is defined - do we still want to reference the kinetic s3 bucket or are all the images officially local now with your placeholder addition here?
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.
getImageUrl is defined in the card-images.tsx file which is inside the study-cards-images component folder. The kinetic s3 bucket is no longer needed.
I also noticed that, It would fail if the image is a png. There a couple of png images in the public folder. |
Great job! Feel free to merge this @Coder-Srinivas |