Fix : [#2488] Edit svg file to resolve the issue. #2494
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. |
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-096a53e88e |
client/images/p5js-logo-small.svg
Outdated
| @@ -1,13 +1,13 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="56" height="42" viewBox="0 0 56 42"> | |||
| <defs> | |||
| <path id="a" d="M0 0h36v36H0z"/> | |||
| <path id="a" d="M1 0h34v35H1z"/> | |||
There was a problem hiding this comment.
Do you think that we need any of these rectangular paths? It seems like both a and b can be deleted as well. This SVG is definitely not optimized and the more that I look at the source the more I hate it!
There was a problem hiding this comment.
It seems like we 🗑️ b and both mask commands. But we have to keep a because the paths for the logo are inverted, like it's the path for the background around the "p5*" rather than the path for the "p5*". Which annoys me but oh well, probably not worth fixing. https://graphicdesign.stackexchange.com/a/84312/122121
There was a problem hiding this comment.
Even though I know it's not worth fixing I can't stop myself 😆 Had to see if I could do it. This is cleaner, though idk if it will create any issues with CSS targeting.
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="56" height="42" viewBox="0 0 56 42">
<g fill="none" fill-rule="evenodd">
<path fill="#ED225D" d="M0 0h56v42H0z"/>
<path fill="#FFFFFF" d="M42.75 16.44l-.989.717-1.026-1.361-1.001 1.326-.963-.734.975-1.337-1.582-.571.374-1.15 1.586.511v-1.644h1.273v1.647l1.574-.476.373 1.15-1.57.536.975 1.385zm-7.045 8.959a5.208 5.208 0 0 1-1.25 1.769c-.524.481-1.139.85-1.844 1.105a6.559 6.559 0 0 1-2.25.383c-1.334 0-2.46-.312-3.374-.936a5.238 5.238 0 0 1-1.533-1.614l1.953-1.835.03-.01a3.14 3.14 0 0 0 1.125 1.52c.525.384 1.147.576 1.867.576.405 0 .787-.068 1.147-.204.36-.134.675-.33.945-.585.27-.256.484-.575.641-.958.158-.383.237-.815.237-1.296 0-.616-.106-1.127-.315-1.533a2.775 2.775 0 0 0-.821-.98 3.075 3.075 0 0 0-1.136-.519 5.634 5.634 0 0 0-1.282-.146c-.3 0-.619.022-.957.067-.337.046-.667.11-.99.192-.322.083-.637.177-.944.281a6.717 6.717 0 0 0-.821.338l.27-8.722h9.065v2.434h-6.501l-.157 3.449c.254-.075.558-.128.91-.158.353-.03.672-.045.957-.045.78 0 1.503.112 2.17.338a5.15 5.15 0 0 1 1.744.98c.495.429.88.955 1.158 1.578.278.624.416 1.341.416 2.153 0 .887-.153 1.679-.46 2.378zm-11.601-.36a5.504 5.504 0 0 1-1.058 1.825 5.08 5.08 0 0 1-1.664 1.24c-.653.307-1.392.462-2.216.462a4.678 4.678 0 0 1-2.103-.474c-.638-.315-1.13-.743-1.474-1.285h-.045v6.853h-2.7V17.43h2.588v1.51h.067c.135-.21.311-.424.529-.642.217-.218.484-.413.799-.587a4.908 4.908 0 0 1 2.407-.597c.764 0 1.469.147 2.114.44.645.293 1.2.695 1.665 1.206a5.465 5.465 0 0 1 1.08 1.803c.254.691.382 1.435.382 2.231 0 .797-.124 1.544-.371 2.243z M21.025 20.617a2.826 2.826 0 0 0-.941-.795c-.38-.201-.824-.302-1.334-.302-.481 0-.912.104-1.29.314-.38.209-.704.478-.974.805-.27.329-.474.702-.612 1.12a3.85 3.85 0 0 0 0 2.496c.138.411.342.78.612 1.109.27.328.594.593.974.794.378.202.809.302 1.29.302.51 0 .955-.104 1.334-.313.38-.209.693-.477.94-.806.248-.328.434-.701.559-1.12a4.283 4.283 0 0 0 0-2.497c-.125-.409-.31-.778-.558-1.107"/>
</g>
</svg>
There was a problem hiding this comment.
@lindapaiste Thank you for your guidance, I have updated the file and your method is elegant and optimized.
I have also updated the related .css file according to the new svg updates, so it won't cause any trouble. Can you please review my pr.
There was a problem hiding this comment.
Awesome! Here's an idea. Instead of using a path for the background we can use a rect (<rect fill="#ED225D" x="0" y="0" width="56" height="42"/>). That would make the CSS targeting easier. We would have a rect and a path instead of two paths so we wouldn't need to to use nth-child selectors.
Or...maybe this is better. We could remove the background from the SVG file entirely and set the background color using CSS.
There was a problem hiding this comment.
Yeah that sounds good, we should use CSS to set the color of the background that would simplify the svg and also reduce it's size. I have made the necessary changes, you can review.
There was a problem hiding this comment.
Hey @lindapaiste meanwhile, I found a one more little bug
In first image, when we hover over the toggle explorer button the cursor change to horizontal scroller, but it should be a pointer as shown in the second image. So, should I create an another issue on this or should I just edit the current issue and add this little bug, and just update my pull request by squashing the two commits.
There was a problem hiding this comment.
Hey @lindapaiste, just a reminder.
There was a problem hiding this comment.
That would be a separate issue but it's already been reported and we decided to keep it as-is. #2489
| @include themify() { | ||
| // Set internal color of the logo; |
There was a problem hiding this comment.
These comments will be helpful to anyone looking at this. Because now that we inverted the SVG file the naming is kind of backwards isn't it! 'logo-color' is the background and 'logo-background-color' is the text 😆 .
Fixes #2488
Changes:
Edited the svg that contains the logo (p5js-logo-small) of the resultant issue.
I have verified that this pull request:
npm run lint)npm run test)