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 battery-full icon #172

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

M-arcus
Copy link

@M-arcus M-arcus commented Aug 25, 2017

No description provided.

@hamptonmoore
Copy link

This would be a great inclusion

Ecleptic
Ecleptic previously approved these changes Jan 22, 2019
Copy link

@Ecleptic Ecleptic left a comment

Choose a reason for hiding this comment

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

It looks good to me!

Here's a preview:
image

@locness3
Copy link

#171 Guidelines says "no fills"

@Ecleptic
Copy link

Looks like I completely missed that in the guidelines. I'll re-build it.
I need a battery in my current project.

hamptonmoore
hamptonmoore previously approved these changes Jan 27, 2019
@ashygee
Copy link
Collaborator

ashygee commented Jan 28, 2019

@herohamp curious why this is approved? the icon still contains a fill which is against the guidelines (#171) pointed out by @locness3.

@M-arcus M-arcus dismissed stale reviews from hamptonmoore and Ecleptic via 27ef596 January 28, 2019 11:57
@M-arcus
Copy link
Author

M-arcus commented Jan 28, 2019

@herohamp @ashygee I have changed the svg

@codecov
Copy link

codecov bot commented Jan 28, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@d2ea756). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             master   #172   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      5           
  Lines             ?     34           
  Branches          ?      3           
=======================================
  Hits              ?     34           
  Misses            ?      0           
  Partials          ?      0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2ea756...27ef596. Read the comment docs.

@ashygee
Copy link
Collaborator

ashygee commented Jan 29, 2019

cc @colebemis

Copy link

@locness3 locness3 left a comment

Choose a reason for hiding this comment

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

They are still fills actually. The rectangles are filled.

@M-arcus
Copy link
Author

M-arcus commented Feb 14, 2019

@locness3 Sorry, I can't see the fill. Only that the fill is set to none.

Could you please point it out?

@locness3
Copy link

@M-arcus Strange, the Github rich diff displays a fill
screenshot_2019-02-15 add battery-full icon by m-arcus pull request 172 feathericons feather

@M-arcus
Copy link
Author

M-arcus commented Feb 15, 2019

@locness3 Probably because the line are thick. Should i make them thinner?

@locness3
Copy link

@locness3 Probably because the line are thick. Should i make them thinner?

Guidelines say (#171) that icons should have a 2px stroke. Check out the guidelines about spacing.

@mittalyashu
Copy link
Contributor

The battery looks great, can you add 1px of spacing around the icon.

Can you create a different version of battery like:

Battery 0 Battery 1 Battery 2 Battery 3
Figma_9Lqtq9H4sE Figma_B31Unm4d5q Figma_uaKYg3v7DM Figma_mrUd1wBnYs

@csandman
Copy link
Contributor

I know this is old, but I wanted to weigh in. I feel like the version by @mittalyashu doesn't quite match the guidelines for 2px wide stroke width's with the bars. I feel like the best solution to this would be to implement a similar shape but using lines instead. Here are my versions that stick with the original battery's style (using rect and line):


battery-full

image

<svg
  xmlns="http://www.w3.org/2000/svg"
  width="24"
  height="24"
  viewBox="0 0 24 24"
  fill="none"
  stroke="currentColor"
  stroke-width="2"
  stroke-linecap="round"
  stroke-linejoin="round"
>
  <rect x="1" y="6" width="18" height="12" rx="2" ry="2" />
  <line x1="23" y1="13" x2="23" y2="11" />
  <line x1="5" y1="14" x2="5" y2="10" />
  <line x1="10" y1="14" x2="10" y2="10" />
  <line x1="15" y1="14" x2="15" y2="10" />
</svg>

battery-medium

image

<svg
  xmlns="http://www.w3.org/2000/svg"
  width="24"
  height="24"
  viewBox="0 0 24 24"
  fill="none"
  stroke="currentColor"
  stroke-width="2"
  stroke-linecap="round"
  stroke-linejoin="round"
>
  <rect x="1" y="6" width="18" height="12" rx="2" ry="2" />
  <line x1="23" y1="13" x2="23" y2="11" />
  <line x1="5" y1="14" x2="5" y2="10" />
  <line x1="10" y1="14" x2="10" y2="10" />
</svg>

battery-low

image

<svg
  xmlns="http://www.w3.org/2000/svg"
  width="24"
  height="24"
  viewBox="0 0 24 24"
  fill="none"
  stroke="currentColor"
  stroke-width="2"
  stroke-linecap="round"
  stroke-linejoin="round"
>
  <rect x="1" y="6" width="18" height="12" rx="2" ry="2" />
  <line x1="23" y1="13" x2="23" y2="11" />
  <line x1="5" y1="14" x2="5" y2="10" />
</svg>

And here are the files:
battery-icons.zip

I could submit a separate pull request, what do you think @colebemis? I know you've been busy so I tried to make these match the current style (code and guidelines) as much as possible to make it easy for you haha.

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.

None yet

7 participants