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 3x3x3 Speed-Blind icon #120

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Add 3x3x3 Speed-Blind icon #120

merged 6 commits into from
Dec 1, 2023

Conversation

dmint789
Copy link
Contributor

Preview:

image

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 2, 2023

@jfly thoughts?

lgarron
lgarron previously requested changes Nov 3, 2023
Copy link
Member

@lgarron lgarron left a comment

Choose a reason for hiding this comment

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

I think this is a pretty good approach, but the white outline on the blindfold needs to be transparent.

Also:

  • Do you think we'll see other speed blindfolded events? I could imagine people holding it for 2x2x2.
  • This differs from the existing stopwatch design (penalty-A4d1). If it's a unique design (and not taken from a potentially copyrighted iconset), that's fine by me (although I might make the lines a bit thicker for small resolutions).

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 3, 2023

I think this is a pretty good approach, but the white outline on the blindfold needs to be transparent.

Also:

  • Do you think we'll see other speed blindfolded events? I could imagine people holding it for 2x2x2.
  • This differs from the existing stopwatch design (penalty-A4d1). If it's a unique design (and not taken from a potentially copyrighted iconset), that's fine by me (although I might make the links a bit thicker for small resolutions).

I doubt we'll ever see 2x2 speedblind, especially since it would be hardly any different from regular 2x2. I thought about making it a more generic icon, but then there'd be way too much detail.

I actually really dislike the way the stopwatch looks in penalty-A4d1, and would rather not use it for this icon. This is a unique design I made, based on inspiration from some examples I found online, which is a normal practice. I can edit it though, that's no problem.

I'll try to fix the outline too, once we fully agree on the other points.

Copy link
Member

@jfly jfly left a comment

Choose a reason for hiding this comment

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

LGTM, modulo the white outline.

I'm also supportive of updating cubing-icon penalty-A4d1 with whatever stopwatch design you land on here.

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 3, 2023

So am I good to just fix the outline and make the lines like 30% thicker?

@jfly
Copy link
Member

jfly commented Nov 3, 2023

That works for me

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 4, 2023

I've made the lines thicker, but I can't figure out how to make the outline transparent. Please advise.

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 6, 2023

@jfly ping

@jfly
Copy link
Member

jfly commented Nov 7, 2023

@dmint789 sorry for the delay. I've made the border transparent. I'm not sure if it's the cleanest way of accomplishing this or not, I am far from an Inkscape expert.

@jfly jfly requested a review from lgarron November 7, 2023 21:27
@dmint789
Copy link
Contributor Author

dmint789 commented Nov 9, 2023

@jfly can you please tell me how you did it? I'd like to know for the future. Is it the same as the borders on multi blind old style? I dunno how those were done either though.

@jfly
Copy link
Member

jfly commented Nov 9, 2023

I duplicated the blindfold, and used inkscape's clipping functionality. This was a bit tricky for me because I needed to get rid of the stroke to make this work (which I think I did by converting the object to a path).

@dmint789
Copy link
Contributor Author

dmint789 commented Nov 9, 2023

Oh, the stroke is actually quite easy to disable. You mind if I try again? Maybe I'll come up with something more elegant. I'll use that clipping function you mentioned.

@jfly
Copy link
Member

jfly commented Nov 9, 2023

You mind if I try again?

Please, go for it!

@dmint789
Copy link
Contributor Author

@jfly I've fixed it now. Do you want me to update the stopwatch in that other icon within the scope of this PR too?
@lgarron please review the changes

@jfly
Copy link
Member

jfly commented Nov 14, 2023

Do you want me to update the stopwatch in that other icon within the scope of this PR too?

Sure, go for it!

@dmint789
Copy link
Contributor Author

@jfly done. Please review and make a new version of the npm package with this icon and the new Guildford icons.

@lgarron
Copy link
Member

lgarron commented Nov 16, 2023

I definitely like the design, but there are a few changes I'd make:

  • Convert the round part of the stopwatch to a path outline rather than a path stroke, to ensure that the SVG coloring works consistently with other icons for any non-iconfont use cases.
  • Remove unused stroke data.
  • Minify using svgo/SVGOMG.

I was a bit concerned about the use of a clip path, but it looks like like this has wide support in browsers and SVG tools.

@dmint789
Copy link
Contributor Author

@lgarron I think I can do the first two, but I don't get the third point. How do I minify it, and what's the purpose of doing that?

@lgarron
Copy link
Member

lgarron commented Nov 17, 2023

How do I minify it, and what's the purpose of doing that?

The simplest is probably to use https://jakearchibald.github.io/svgomg/

Smaller file sizes can help a page load more quickly over the internet. We encourage people to use the icon font, but if anyone used the SVGs that could definitely help load times with pages that are essentially just text and icons (e.g. https://unofficial.cubing.net/).

@jfly
Copy link
Member

jfly commented Nov 17, 2023

We have never asked people to minify SVGs in the past. If we care about this, I'd rather we add it as a build step.

@dmint789
Copy link
Contributor Author

Yeah, I agree (and not just because that's less work for me, haha)

@dmint789
Copy link
Contributor Author

@lgarron I have removed all of the stroke data (at least I think I did), and transformed the circle into a path.

@dmint789
Copy link
Contributor Author

@lgarron ping :)

svgs/penalty/A4d1.svg Outdated Show resolved Hide resolved
@dmint789
Copy link
Contributor Author

@jfly since @lgarron isn't responding, could you cancel his blocking review?

@dmint789
Copy link
Contributor Author

dmint789 commented Dec 1, 2023

@jfly is this good to merge now? I've reverted the changes to the penalty icon.

@jfly jfly dismissed lgarron’s stale review December 1, 2023 07:32

I'm happy with dmint789's work

@jfly jfly merged commit 71e0f25 into cubing:main Dec 1, 2023
1 check passed
@dmint789
Copy link
Contributor Author

dmint789 commented Dec 1, 2023

@jfly thank you. Can you please release a new NPM package with both this and the other new icons?

@jfly
Copy link
Member

jfly commented Dec 5, 2023

@dmint789 version 1.1.2 is released!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants