Skip to content

Better arrows#30

Open
JorenC wants to merge 70 commits intomainfrom
betterArrows
Open

Better arrows#30
JorenC wants to merge 70 commits intomainfrom
betterArrows

Conversation

@JorenC
Copy link
Collaborator

@JorenC JorenC commented Feb 12, 2025

Adding the PR now, rather than make it a huge thing.

  • Added a lot of setup to the stories so we can see more orders - this is a real game.
  • Changed arrows so they use the colour of the origin nation.
  • Changed initial values of the arrows
  • Added different fail states
  • Changes the '+' to 'x' on the orders (you passed "angle="angle" but I think it was nowhere defined, I now hardcoded it to 45)
  • Included support hold order
  • Included convoy order

johnpooch and others added 30 commits January 4, 2025 11:14
Update favicon and change document title to Diplicity
Add phase select functionality to game details screens
Update README to reflect project details and setup instructions
Fix issue with selectedPhase deleting all qs params
…ion; implement tests and update order handling
…onsolidate logic in useFindGames and useMyGames
…ructure and add logo image; update My Games image size
…etailAppBar with a divider and improve orders display message
…nent for displaying orders alongside the game map
/rgb(a?)\((\d+), (\d+), (\d+)(, [\d.]+)?\)/,
// "rgba($2, $3, $4, 0.4)"
`rgba($2, $3, $4, ${isSelected ? 0.3 : isHovered ? 0.4 : 0.5})`
`rgba($2, $3, $4, ${isSelected ? 0.3 : isHovered ? 0.4 : 0.5})`,
Copy link
Owner

Choose a reason for hiding this comment

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

Tiny thing, and we can pick this up in a future iteration, but this 0.3, 0.4, and 0.5 should be extracted into named constants like PROVINCE_COLOR_OPACITY_HOVERED.

const source = props.map.provinces.find((p) => p.id === provinceId);
const target = props.map.provinces.find((p) => p.id === order.target);
const unit = props.units[provinceId];
const unitColor = unit ? props.nationColors[unit.nation] : "black"; // Default to black if no unit found
Copy link
Owner

Choose a reason for hiding this comment

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

Curious, can this happen in practice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think really, unless it's a bug - but it helps identify issues when using StoryBook and is a nice fallback

width={ORDER_FAILED_CROSS_WIDTH}
length={ORDER_FAILED_CROSS_LENGTH}
angle={angle}
angle={45}
Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, I saw your comment about this. So the angle value that gets passed to this callback is the angle of the arrow being drawn on, so my thinking was we would either do angle or angle + 45 or something. But if what you have works, that's fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's leave it at this then.

const source = props.map.provinces.find((p) => p.id === provinceId);
const target = props.map.provinces.find((p) => p.id === order.target);
const unit = props.units[provinceId];
const unitColor = unit ? props.nationColors[unit.nation] : "black"; // Default to black if no unit found
Copy link
Owner

Choose a reason for hiding this comment

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

I'll do a subsequent iteration to tidy up some of this repeated code and stuff. I think I can restructure this file a little more nicely.

};

const centerPoint = getBezierPoint(0.5, start, cp1, cp2, end);
const centerPoint = getBezierPoint(0.3, start, cp1, cp2, end);
Copy link
Owner

Choose a reason for hiding this comment

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

Is this something like the "amount" of curve? I think we should pass it in as a prop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this is where the 'x' gets drawn on the support curve.

If we do this at 0.5, this means that it usually overlaps with the actual move order. If we set it at about 0.3, it usually is where the support order does not yet overlap the actual move order. We could pass it as a prop, but I don't think we'll ever change this.

);
};

export { HoldArrow };
Copy link
Owner

Choose a reason for hiding this comment

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

Let's rename this component to be just Line/line.tsx. Let's give the responsibility of rendering the octagon for the supported unit to interactive-map.tsx

This shapes subfolder is really about primitive shapes, and shouldn't have game logic built in.

Also, it saves us having to pass in the octagon values as props, which will make this component more messy.

So basically, for every "support hold" order, draw a Line component and an Octagon component

Copy link
Owner

@johnpooch johnpooch left a comment

Choose a reason for hiding this comment

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

@JorenC I'll take a look at this tomorrow or the day after and implement the feedback I've left as well as doing some other general tidy-up.

Tokenized the values. I'll try to keep this best practice - still not in my system as I design using the interface and then just believe that should be the value :D
Repository owner deleted a comment from JorenC Oct 11, 2025
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