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

chore: another pixel placement control flow #206

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vibenedict
Copy link

Clicking on a pixel in the canvas should open the pixel color selector (if "Place Pixel" button is active)

Copy link

vercel bot commented Jun 1, 2024

@vibenedict is attempting to deploy a commit to the keep-starknet-strange Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Contributor

@b-j-roberts b-j-roberts left a comment

Choose a reason for hiding this comment

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

How this is implemented now, here is the user's flow:

  1. Click the pixel on the canvas when one is available
  2. Click to select a color from the palette
  3. Click the same pixel again on the canvas to color it

For a better user experience I think something like this would be better

  1. Click a pixel on the canvas when one is available
  2. Click a color on the palette
    a. If the user only has one pixel available, submit to color the pixel selected with the color chosen
    b. If the user has more than one pixel available, color the pixel the user selected and select the color to use for extra pixels.

@@ -303,6 +305,11 @@ function App() {
setSelectedPositionX(x);
setSelectedPositionY(y);
setPixelSelectedMode(true);

if (placementTimer == 'Place Pixel') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Checking against a string here would be bad practice because the string can change. For instance, when the user has multiple pixels, the button says 'Place Pixels' instead.

I'd recommend using availablePixels > 0 for this check.

@b-j-roberts
Copy link
Contributor

@vibenedict Will you be able to make these changes?

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.

2 participants