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

Dog-ears to show symmetrical cell #453

Merged
merged 7 commits into from
Sep 19, 2023
Merged

Conversation

adamthedog
Copy link
Member

Cells which are symmetrical get a little yellow triangle in the corner to help with blocking.

Copy link
Member

@mdirolf mdirolf left a comment

Choose a reason for hiding this comment

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

Overall looks great! Thank you! I commented with a couple of minor nits and one or two things that might need a change or clarification.

@@ -557,8 +557,7 @@ export const Builder = (props: BuilderProps & AuthProps): JSX.Element => {
isPrivate: state.isPrivate,
isPrivateUntil: state.isPrivateUntil?.toMillis(),
alternates: state.alternates,
userTags: state.userTags,
symmetry: state.symmetry,
Copy link
Member

Choose a reason for hiding this comment

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

Was this just an accidental change? I think the effect is to not store the current symmetry setting between sessions.

Copy link
Member Author

Choose a reason for hiding this comment

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

forgot to revert that as well from when I was using F5 as a symmetry reset button

@@ -134,7 +139,7 @@ export const GridView = ({
gridWidth={grid.width}
gridHeight={grid.height}
active={isActive}
entryCell={entryCells.some((p) => cellIndex(grid, p) === idx)}
entryCell={entryCell}
Copy link
Member

@mdirolf mdirolf Sep 18, 2023

Choose a reason for hiding this comment

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

No big deal but I have a slight preference for undoing the change that shifted entryCell into its own var - just to keep this PR's surface area as small as possible. I wonder if you had used it in one of your other iterations so needed to pull it out for that

@@ -119,6 +120,10 @@ export const GridView = ({
const col = idx % grid.width;
const row = Math.floor(idx / grid.height);

const entryCell = entryCells.some((p) => cellIndex(grid, p) === idx);
const symmetricalCell = (props.symmetry != Symmetry.None) ? flipped(grid, active, props.symmetry as Symmetry) : null;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to check for the case that props.symmetry is null/undefined here? I.e. in the case that this grid is being used for a puzzle rather than the builder?

Copy link
Member

Choose a reason for hiding this comment

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

I think that would enable us to get rid of the as cast, too - which should always be a last resort.

Copy link
Member Author

Choose a reason for hiding this comment

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

shoot I addressed the null case but i had rewritten the line after ... well, college football game 😆
For whatever reason, it does work as-is in a puzzle, but you're right that it should be left in

Copy link
Member

Choose a reason for hiding this comment

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

😂

app/lib/style.ts Outdated
return {
'--tag-l': darkMode ? '30%' : '85%',
'--bg': bg,
'--primary': p,
'--blue': darkMode ? mix('blue', 'white', 0.5) : 'blue',
'--green': darkMode ? mix('green', 'white', 0.5) : 'green',
'--onprimary': readableColor(p, darkMode),
'--lighter': mix(p, cellBG, 0.6),
'--lighter': lighter,
Copy link
Member

Choose a reason for hiding this comment

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

similar to the entryCell comment above - I'm OK with this but have an ever so slight pref for undoing the change since it doesn't seem to be a necessary part of the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

ah right, I missed those changes when I reviewed but knew I was forgetting something.

@mdirolf mdirolf merged commit 4f9df4c into crosshare-org:master Sep 19, 2023
1 check failed
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