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

Add warning if user is creating scratch with the same name as an exis… #1426

Merged
merged 4 commits into from
Feb 2, 2025

Conversation

mkst
Copy link
Collaborator

@mkst mkst commented Feb 1, 2025

…ting one...

image

Styling can be tweaked if necessary, but i think this does the job. Only returns 10 (page_size) results, but we can tweak that if it becomes an issue.. this is just meant to improve the UX and prevent two people from working on the same function for the same game without realising :)

Closes #1425.

Comment on lines 287 to 291
get(`/scratch?search=${label}`)
.then((x) => x.results)
.then(filterCandidates)
.then(setDuplicates);
}, [label, platform, presetId]);
Copy link
Member

Choose a reason for hiding this comment

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

Since label is a text input, every character the user types into it will trigger a network request, which we don't want. I'd probably wrap use useDebounce, like this code that will only make the api call 1000ms after the user finishes typing:

const [debouncedContext] = useDebounce(scratch.context, 1000, {
leading: false,
trailing: true,
});
const [valueVersion, setValueVersion] = useState(0);
const url = scratchUrl(scratch);
useEffect(() => {
api.post(`${url}/decompile`, {
context: debouncedContext,
compiler: scratch.compiler,
}).then(({ decompilation }: { decompilation: string }) => {
setDecompiledCode(decompilation);
setValueVersion((v) => v + 1);
});
}, [scratch.compiler, debouncedContext, url]);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whilst label is editable by humans, it's most likely auto-populated based on the target asm being pasted in.. but yeh I can wrap it in a debounce to avoid spamming the API if someone starts typing there 👍

@@ -113,3 +113,14 @@
}
}
}

.duplicatesContainer {
Copy link
Member

Choose a reason for hiding this comment

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

Probably use tailwind instead given #1394

Comment on lines +288 to +291
return (
scratch.owner &&
(!api.isAnonUser(scratch.owner) ? (
userAvatarUrl(scratch.owner) && (
Copy link
Member

Choose a reason for hiding this comment

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

This could be more clearly written without ternaries: if (scratch.owner && ...) { return <Image /> } else { return <Frog /> }. Great idea for a component

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 just lifted this code verbatim from the Search code, but yeh that makes sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can't appease the linter so gonna leave this as-is.

@ethteck ethteck merged commit 3fe7a97 into main Feb 2, 2025
7 checks passed
@ethteck ethteck deleted the warn-on-dupe-scratch-names branch February 2, 2025 09:46
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.

Tell user if a scratch with the same name already exists
3 participants