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

feat: add box select #69

Conversation

llaenowyd
Copy link
Contributor

@llaenowyd llaenowyd commented Sep 6, 2020

mishkaio_box_select_grid

mishkaio_box_select_1_1

Note that the original version of the above was first submitted by @mishkaio under #57.

@mishkaio

This comment has been minimized.

@johnletey

This comment has been minimized.

@ericfennis
Copy link
Member

I reopened this issue, I don't get any response on #57 .
So @llaenowyd feel free to submit your versions.

@llaenowyd
Copy link
Contributor Author

I reopened this issue, I don't get any response on #57 .
So @llaenowyd feel free to submit your versions.

Ok. I'm completely busy through the end of this week.

I took a look at the other PR. I think the latest feedback is to try something more of a dashed-line rectangle.

What I might do is focus the change down to the selection rectangle.

Is the version with cursor semantically different or a stylistic option?

Unless I'm missing something about it, I might bring in the box-with-cursor as an alternative possibility for box-select, rather than a request for 2 icons for box-select.

@ericfennis
Copy link
Member

@llaenowyd No problem, take your time!

@llaenowyd llaenowyd force-pushed the mishkaio_pr_57_lasso_box_select_hack_job branch from 2eb7154 to 7280921 Compare September 26, 2020 13:10
@llaenowyd llaenowyd changed the title feat: add box select, box select with pointer, lasso select, lasso feat: add box select Sep 26, 2020
@llaenowyd
Copy link
Contributor Author

The latest review feedback was when @ericfennis said

Btw we can also go more this way? ...
image

@llaenowyd
Copy link
Contributor Author

dashed_line_square_grid
dashed_line_square_1_1

Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

@llaenowyd Nice work, looks awesome!!
I think we can optimize the code a bit by replacing some line elements with polylines.

icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
Copy link
Member

@ericfennis ericfennis left a comment

Choose a reason for hiding this comment

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

@llaenowyd We always add an space before the end of the tag. Sorry I missed those.

icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
icons/box-select.svg Outdated Show resolved Hide resolved
@llaenowyd
Copy link
Contributor Author

np - missed that - updated

@ericfennis
Copy link
Member

I checked the guidelines, I think we forgot one thing.
We have 2px rounded corners.

What do you think? I think this looks better.

image
100%
image

Here is the code

<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"
>
  <path d="M4 2a2 2 0 00-2 2" />
  <line x1="8" y1="2" x2="10" y2="2" />
  <line x1="14" y1="2" x2="16" y2="2" />
  <path d="M4 22a2 2 0 01-2-2" />
  <line x1="22" y1="8" x2="22" y2="10" />
  <line x1="22" y1="14" x2="22" y2="16" />
  <path d="M22 20a2 2 0 01-2 2" />
  <line x1="14" y1="22" x2="16" y2="22" />
  <line x1="8" y1="22" x2="10" y2="22" />
  <path d="M20 2a2 2 0 012 2" />
  <line x1="2" y1="14" x2="2" y2="16" />
  <line x1="2" y1="8" x2="2" y2="10" />
</svg>

@moeenio
Copy link
Contributor

moeenio commented Sep 28, 2020

Yup, rounded looks better.

@llaenowyd
Copy link
Contributor Author

same, looks good!

@llaenowyd
Copy link
Contributor Author

🎉

I think I can consider this done for my part right?

What's the rough estimated timeline for it showing up in npm feather-icons?

@ericfennis
Copy link
Member

@llaenowyd unfortunately maybe never.
The author of feather-icons stopped maintaining the feather-icons projects 😔.
Featherity is just an independent fork runned by the community some of people worked on Feather icons.
See this thread, how it all started.

@ericfennis ericfennis merged commit c84fcfe into lucide-icons:master Sep 29, 2020
@ericfennis
Copy link
Member

@llaenowyd But we are launching our own package soon!

@llaenowyd
Copy link
Contributor Author

Ok, I see, interesting. I've seen dead packages and live packages, but I've never watched what looks to be a well motivated community effort to resuscitate a package. Hope to see it land. 👍

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.

5 participants