Feat/custom menu context #313
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bryanlundberg
requested changes
Jun 19, 2024
There was a problem hiding this comment.
Hi @syrok94 thank you for your efforts on this implementation. I have a few points of feedback:
-
Visual Implementation: The current visual design does not match the desired look. There are some unexpected behaviors, and the component is not responsive. Additionally, the colors do not align with our specifications.
-
Code Reusability: It would be great if you could reuse the existing code for this feature. The necessary section already exists, so please refactor and integrate it into the component.
Thank you for addressing these issues. If you have any questions, feel free to reach out.
Comment on lines
+53
to
+55
| useEffect(() => { | ||
| setContextMenu({ ...contextMenu, visible: false }); | ||
| }, [status]); |
There was a problem hiding this comment.
Here drops a warning at build time
Comment on lines
+67
to
+94
| const handleMove = async (solve: Solve, currentTab: SolveTab) => { | ||
| if (!selectedCube) return; | ||
| const updatedCube = await moveSolve({ | ||
| solve, | ||
| selectedCube, | ||
| type: currentTab, | ||
| }); | ||
| mergeUpdateSelectedCube(updatedCube, cubes); | ||
| setStatus(false); | ||
| }; | ||
|
|
||
| const handleDelete = async (solve: Solve) => { | ||
| if (!selectedCube) return; | ||
| const updatedCube = await updateSolve({ | ||
| selectedCube: selectedCube, | ||
| solveId: solve.id, | ||
| type: "DELETE", | ||
| }); | ||
| mergeUpdateSelectedCube(updatedCube, cubes); | ||
| setStatus(false); | ||
| }; | ||
|
|
||
| const handleCopyToClipboard = async (text: string) => { | ||
| if ("clipboard" in navigator) { | ||
| await navigator.clipboard.writeText(text); | ||
| } | ||
| setStatus(false); | ||
| }; |
There was a problem hiding this comment.
This logic already exist in source, no need duplicate.
Comment on lines
+154
to
+210
| {contextMenu.visible && | ||
| contextMenu.solve === displaySolves[index] && ( | ||
| <AnimatePresence> | ||
| <motion.div | ||
| initial={{ y: 0, scale: 0.9, opacity: 0 }} | ||
| animate={{ y: 0, scale: 1, opacity: 1 }} | ||
| exit={{ x: 0, scale: 0.9, opacity: 0 }} | ||
| ref={submenuRef} | ||
| className="absolute flex flex-col w-32 gap-3 py-2 bg-white rounded-md" | ||
| style={{ | ||
| top: `${contextMenu.y - window.scrollY}px`, | ||
| left: `${contextMenu.x - window.scrollX}px`, | ||
| zIndex: 50, | ||
| }} | ||
| > | ||
| <div | ||
| className="flex items-start justify-start gap-1 py-1 transition duration-200 ps-2 hover:text-neutral-500 hover:cursor-pointer" | ||
| onClick={() => | ||
| currentTab === "Session" | ||
| ? handleMove(contextMenu.solve!, "Session") | ||
| : handleMove(contextMenu.solve!, "All") | ||
| } | ||
| > | ||
| <div className="w-4 mr-3 h-4"> | ||
| <ArchiveBoxArrowDownIcon className="w-6 h-6" /> | ||
| </div> | ||
| <div> | ||
| {currentTab === "Session" ? t("archive") : t("unarchive")} | ||
| </div> | ||
| </div> | ||
| <div | ||
| className="flex items-start justify-start gap-1 py-1 transition duration-200 ps-2 hover:text-neutral-500 hover:cursor-pointer" | ||
| onClick={() => | ||
| handleCopyToClipboard( | ||
| `[${formatTime(contextMenu.solve!.time)}s] - ${ | ||
| contextMenu.solve!.scramble | ||
| }` | ||
| ) | ||
| } | ||
| > | ||
| <div className="w-4 mr-3 h-4"> | ||
| <DocumentDuplicateIcon className="w-6 h-6" /> | ||
| </div> | ||
| <div>{t("copy")}</div> | ||
| </div> | ||
| <div | ||
| className="flex items-start justify-start gap-1 py-1 transition duration-200 ps-2 hover:text-neutral-500 hover:cursor-pointer" | ||
| onClick={() => handleDelete(contextMenu.solve!)} | ||
| > | ||
| <div className="w-4 mr-3 h-4"> | ||
| <TrashIcon className="w-6 h-6" /> | ||
| </div> | ||
| <div>{t("remove")}</div> | ||
| </div> | ||
| </motion.div> | ||
| </AnimatePresence> | ||
| )} |
There was a problem hiding this comment.
This small mini section, already exist as solve options, no need duplicate
Comment on lines
+243
to
244
| className="flex items-start justify-start gap-1 py-1 transition duration-200 ps-2 hover:text-neutral-500 hover:cursor-pointer" | ||
| onClick={() => |
|
Merged: #316 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fix #312 and added Feat #270 which results in adding a context menu.
Related Issue(s)
None
Changes
Screenshots or GIFs (if applicable)
Additional Comments
If some changes have to be made, please let me know.
By submitting this PR, I confirm that: