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

menu cleanup #1: tagging in tags column, metas in name column #848

Merged
merged 2 commits into from
Jan 15, 2025

Conversation

npinsker
Copy link
Collaborator

@npinsker npinsker commented Jan 15, 2025

move 'tag' button to tags column, which is more intuitive.

i didn't bother to add hover behavior to the tag button because i'm planning to remove these icons altogether, but am splitting the commit up a bit.

almost all of this is just c/p between different files, the work was basically already done for me

image

move meta-parenting stuff to the puzzle edit modal and remove it from the tag modal

image image

@npinsker npinsker changed the title move tagging to the tags column, move meta-structuring to the name column menu cleanup #1: move tagging to the tags column, move meta-structuring to the name column Jan 15, 2025
@npinsker npinsker changed the title menu cleanup #1: move tagging to the tags column, move meta-structuring to the name column menu cleanup #1: tagging in tags column, metas in name column Jan 15, 2025
@@ -29,6 +31,16 @@ function EditPuzzleModal({
const [newIsMeta, setNewIsMeta] = React.useState(isMeta);
const [createChannels, setCreateChannels] = React.useState(hasChannels);
const dispatch = useDispatch<Dispatch>();

const huntTags = useSelector(selectHuntTags);
Copy link
Collaborator Author

@npinsker npinsker Jan 15, 2025

Choose a reason for hiding this comment

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

i just c/p this from another file but i wonder if it can be simplified -- what are hunt tags vs. puzzle tags?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think they're actually the same type of object, but hunt implies "all" while puzzle implies individual tags.

More detail: The backend defines PuzzleTags and uses that nomenclature. Each of these PuzzleTags is associated with a single Hunt. The frontend also uses the PuzzleTag nomenclature, but also calls them huntTags like this:

export const selectHuntTags = createSelector(

We can probably just call them tags in the frontend instead of puzzleTag and allTags instead of huntTags since we don't have any other notion of tags in Cardboard AFAIK.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is there any difference between the “hunts tags” derived that way and the “puzzle tags” from

export const selectPuzzleTags = createSelector(
? (in the edit tags modal, those two sets are merged — do they need to be?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so this huntTags thing is the set of all the tags in the hunt at the time when we loaded the hunt object (which is only once at page load). The puzzleTags is the set of all the tags that are currently in use on a puzzle. These two sets are a bit distinct--huntTags will contain all the default tags even if they aren't in use, while puzzleTags will contain any tags that were created after the page was loaded (including meta tags).

I think combining to only a single source would mean adding some more polling just for tags which I might be a little apprehensive about although it's conceptually a lot cleaner.

I think this code isn't going to work as a result, I think you want puzzleTags and not huntTags. I suspect that if a meta is created after the page is loaded its tag won't appear here.

@npinsker npinsker force-pushed the npinsker/refactor-menus-1 branch 2 times, most recently from 1bcf4b5 to 8d86bbd Compare January 15, 2025 09:39
Copy link
Contributor

@akirabaruah akirabaruah left a comment

Choose a reason for hiding this comment

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

i didn't bother to add hover behavior to the tag button because i'm planning to remove these icons altogether, but am splitting the commit up a bit.

Yeah, with the new arrangement, changing the tag icon to a plus or something would probably make more sense visually, but if we're getting rid of the icon entirely (#851 ?), then LGTM.

@@ -29,6 +31,16 @@ function EditPuzzleModal({
const [newIsMeta, setNewIsMeta] = React.useState(isMeta);
const [createChannels, setCreateChannels] = React.useState(hasChannels);
const dispatch = useDispatch<Dispatch>();

const huntTags = useSelector(selectHuntTags);
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah so this huntTags thing is the set of all the tags in the hunt at the time when we loaded the hunt object (which is only once at page load). The puzzleTags is the set of all the tags that are currently in use on a puzzle. These two sets are a bit distinct--huntTags will contain all the default tags even if they aren't in use, while puzzleTags will contain any tags that were created after the page was loaded (including meta tags).

I think combining to only a single source would mean adding some more polling just for tags which I might be a little apprehensive about although it's conceptually a lot cleaner.

I think this code isn't going to work as a result, I think you want puzzleTags and not huntTags. I suspect that if a meta is created after the page is loaded its tag won't appear here.

@npinsker npinsker force-pushed the npinsker/refactor-menus-1 branch from 8d86bbd to 30930fd Compare January 15, 2025 21:50
@npinsker
Copy link
Collaborator Author

I verified that your bug happened, changed code to use puzzleTags, and verified it no longer happens

Copy link
Contributor

@rgossiaux rgossiaux left a comment

Choose a reason for hiding this comment

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

Great ty. Prolly a good indicator that the underlying code could use some improvements (naming and/or commenting at the least).

@npinsker npinsker merged commit 831bfbf into main Jan 15, 2025
2 checks passed
@npinsker npinsker deleted the npinsker/refactor-menus-1 branch January 15, 2025 22:04
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.

3 participants