-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feat/add tag binding #90
Changes from all commits
b3e7ca6
e46f575
29d497b
bb7717c
1d49193
bbdd338
d09e3a9
f2a7b30
20a1eff
9bcb131
2b55f4a
e19a887
0b3c3c6
b9af47d
5e4824f
72f975e
c64b9ee
39034c6
4a22a4e
e800c1b
badbd1d
56006ed
dec6e0e
8de6f1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there any reason that this component can't extend your non-icon button? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The style of both components are a bit different: this one has grey scale colours and is a circle. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import { IconDefinition } from "@fortawesome/free-solid-svg-icons"; | ||
import { FontAwesomeIcon } from "@fortawesome/react-fontawesome"; | ||
import { MouseEventHandler } from "react"; | ||
|
||
type ButtonIconProps = { | ||
onClick: MouseEventHandler<HTMLButtonElement>; | ||
icon: IconDefinition; | ||
}; | ||
|
||
export const ButtonIcon = ({ onClick, icon }: ButtonIconProps) => { | ||
return ( | ||
<button | ||
className="bg-white hover:bg-theodo-grey-regular outline outline-gray-500 text-gray-600 rounded-3xl h-6 w-6 justify-center align-middle transition-colors duration-100" | ||
onClick={onClick} | ||
> | ||
<FontAwesomeIcon icon={icon} /> | ||
</button> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
import { faTimes } from "@fortawesome/free-solid-svg-icons"; | ||
import { Dialog, Transition } from "@headlessui/react"; | ||
import { Fragment, ReactNode } from "react"; | ||
import { ButtonIcon } from "./ButtonIcon"; | ||
import { CardAtom } from "./CardAtom"; | ||
|
||
type PopupProps = { | ||
children: ReactNode; | ||
isOpen: boolean; | ||
close: () => void; | ||
}; | ||
|
||
export const Popup = ({ children, isOpen, close }: PopupProps) => { | ||
return ( | ||
<Transition.Root show={isOpen} as={Fragment}> | ||
<Dialog as="div" className="relative z-10" onClose={() => {}}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. close is a different ticket im assuming? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How do users close the dialog in this case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a button in the top right corner (see screenshot) |
||
<CustomTransition> | ||
<div className="fixed inset-0 bg-gray-500 bg-opacity-75 transition-opacity" /> | ||
</CustomTransition> | ||
<div className="fixed inset-0 z-10 w-screen overflow-y-auto"> | ||
<div className="flex min-h-full items-end justify-center p-4 text-center sm:items-center sm:p-0"> | ||
<CustomTransition> | ||
<Dialog.Panel className="relative transform shadow-xl transition-all sm:my-8 sm:w-full sm:max-w-lg"> | ||
<CardAtom> | ||
<div className="absolute top-3 right-3"> | ||
<ButtonIcon | ||
onClick={close} | ||
icon={faTimes} | ||
/> | ||
</div> | ||
<div className="p-2 flex flex-col"> | ||
{children} | ||
</div> | ||
</CardAtom> | ||
</Dialog.Panel> | ||
</CustomTransition> | ||
</div> | ||
</div> | ||
</Dialog> | ||
</Transition.Root> | ||
); | ||
}; | ||
|
||
const CustomTransition = ({ children }: { children: ReactNode }) => ( | ||
<Transition.Child | ||
as={Fragment} | ||
enter="ease-out duration-300" | ||
enterFrom="opacity-0" | ||
enterTo="opacity-100" | ||
leave="ease-in duration-200" | ||
leaveFrom="opacity-100" | ||
leaveTo="opacity-0" | ||
> | ||
{children} | ||
</Transition.Child> | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,32 @@ | ||
import { TagPopup } from "@/features/tables/components/TagPopup"; | ||
import { Tag } from "@prisma/client"; | ||
import { useState } from "react"; | ||
import { TagLabel } from "../../../components/atoms/TagLabel"; | ||
import { concatAndSortTags, isTagInArray } from "../helpers/tagArrayManagement"; | ||
|
||
export const ColumnContentTags = ({ tags }: { tags: Tag[] }) => { | ||
export const ColumnContentTags = ({ | ||
tags, | ||
logId, | ||
}: { | ||
tags: Tag[]; | ||
logId: string; | ||
}) => { | ||
const [tagArray, setTagArray] = useState<Tag[]>(tags); | ||
const addTag = (tag: Tag) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so this is just setting it locally for now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the idea is to update the database and the UI at the same time so that the user sees the new tag on his log without having to refresh the whole page. To do that I had to store the tags in this state. |
||
if (isTagInArray(tagArray, tag.id)) return; | ||
setTagArray(concatAndSortTags(tagArray, tag)); | ||
}; | ||
return ( | ||
<td className="px-4 py-2 flex flex-wrap"> | ||
{tags.map((tag) => ( | ||
<TagLabel key={tag.id} tag={tag} /> | ||
))} | ||
<td className="px-4 py-2 flex flex-wrap items-center gap-1"> | ||
{tagArray | ||
.map((tag) => <TagLabel key={tag.id} tag={tag} />) | ||
.concat( | ||
<TagPopup | ||
key="plus-button" | ||
logId={logId} | ||
addTagToDisplay={addTag} | ||
/>, | ||
)} | ||
</td> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
import { ButtonIcon } from "@/components/atoms/ButtonIcon"; | ||
import { Popup } from "@/components/atoms/Popup"; | ||
import { TagLabel } from "@/components/atoms/TagLabel"; | ||
import { faPlus } from "@fortawesome/free-solid-svg-icons"; | ||
import { Tag } from "@prisma/client"; | ||
import { useTagPopup } from "../hooks/useTagPopup"; | ||
|
||
type TagPopupProps = { | ||
logId: string; | ||
addTagToDisplay: (tag: Tag) => void; | ||
}; | ||
|
||
export const TagPopup = ({ logId, addTagToDisplay }: TagPopupProps) => { | ||
const { isPopupOpen, openPopup, closePopup, tags, selectTag } = useTagPopup( | ||
logId, | ||
addTagToDisplay, | ||
); | ||
|
||
return ( | ||
<> | ||
<ButtonIcon onClick={openPopup} icon={faPlus} /> | ||
<Popup isOpen={isPopupOpen} close={closePopup}> | ||
<div className="bg-white items-stretch sm:items-start text-center"> | ||
<h2 className=" text-lg font-bold leading-6 text-gray-900 my-4"> | ||
Pick a tag to add | ||
</h2> | ||
<div className=" w-full my-6 p-2 outline outline-gray-500 bg-gray-50 rounded-md px-4 py-2 flex flex-wrap items-center gap-1"> | ||
{tags.map((tag) => ( | ||
<button | ||
key={tag.id} | ||
onClick={() => selectTag(tag.id)} | ||
> | ||
<TagLabel tag={tag} /> | ||
</button> | ||
))} | ||
</div> | ||
</div> | ||
</Popup> | ||
</> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
import "@testing-library/jest-dom"; | ||
import { concatAndSortTags } from "../tagArrayManagement"; | ||
|
||
describe("concatAndSortTags", () => { | ||
const tags = [ | ||
{ id: "1", name: "tag a" }, | ||
{ id: "2", name: "tag c" }, | ||
]; | ||
const tag = { id: "3", name: "tag b" }; | ||
it("should concat the array of tags with the new tag", () => { | ||
const result = concatAndSortTags(tags, tag); | ||
const filteredResult = result.filter((tag) => tag.id === "3"); | ||
expect(filteredResult).toEqual([{ id: "3", name: "tag b" }]); | ||
}); | ||
it("should sort the array of tags", () => { | ||
const result = concatAndSortTags(tags, tag); | ||
expect(result).toEqual([ | ||
{ id: "1", name: "tag a" }, | ||
{ id: "3", name: "tag b" }, | ||
{ id: "2", name: "tag c" }, | ||
]); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import "@testing-library/jest-dom"; | ||
import { findTagById } from "../tagArrayManagement"; | ||
|
||
describe("findTagById", () => { | ||
const tags = [ | ||
{ id: "1", name: "tag1" }, | ||
{ id: "2", name: "tag2" }, | ||
]; | ||
|
||
it("when the id is valid, should return the tag with the given id", () => { | ||
const tagId = "2"; | ||
const tag = findTagById(tags, tagId); | ||
expect(tag).toEqual(tags[1]); | ||
}); | ||
|
||
it("should throw an error if the tag is not found", () => { | ||
const tagId = "3"; | ||
expect(() => findTagById(tags, tagId)).toThrow( | ||
"Failed to find tag of id 3 among the given tags.", | ||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
import { Tag } from "@prisma/client"; | ||
|
||
export const isTagInArray = (tags: Tag[], tagId: string): boolean => { | ||
return tags.some((tag) => tag.id === tagId); | ||
}; | ||
|
||
export const findTagById = (tags: Tag[], tagId: string): Tag => { | ||
const tag = tags.find((tag) => tag.id === tagId); | ||
if (tag === undefined) { | ||
throw new Error( | ||
`Failed to find tag of id ${tagId} among the given tags.`, | ||
); | ||
} | ||
return tag; | ||
}; | ||
|
||
export const concatAndSortTags = (tags: Tag[], newTag: Tag): Tag[] => { | ||
const newTagArray = [...tags, newTag]; | ||
return newTagArray.sort((tagA, tagB) => { | ||
return tagA.name.localeCompare(tagB.name); | ||
}); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
import { LogsAPI } from "@/services/nextAPI/LogsAPI"; | ||
import { TagsAPI } from "@/services/nextAPI/TagsAPI"; | ||
import { Tag } from "@prisma/client"; | ||
import { MouseEventHandler, useEffect, useState } from "react"; | ||
import { findTagById } from "../helpers/tagArrayManagement"; | ||
|
||
export const useTagPopup = ( | ||
logId: string, | ||
addTagToDisplay: (tag: Tag) => void, | ||
) => { | ||
const [isPopupOpen, setIsPopupOpen] = useState(false); | ||
const [tags, setTags] = useState<Tag[]>([]); | ||
|
||
useEffect(() => { | ||
TagsAPI.getTags().then((tags) => setTags(tags)); | ||
}, []); | ||
|
||
const selectTag = async (tagId: string) => { | ||
const tag = findTagById(tags, tagId); | ||
const success = await LogsAPI.connectTagToLog(logId, tagId); | ||
if (success) { | ||
addTagToDisplay(tag); | ||
setIsPopupOpen(false); | ||
} else { | ||
alert("Failed to connect tag to log."); | ||
} | ||
}; | ||
|
||
const openPopup: MouseEventHandler<HTMLButtonElement> = (e) => { | ||
e.stopPropagation(); // Prevents the click event from bubbling up to the table row | ||
setIsPopupOpen(true); | ||
}; | ||
|
||
const closePopup = () => { | ||
setIsPopupOpen(false); | ||
}; | ||
return { isPopupOpen, openPopup, closePopup, tags, selectTag }; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,7 @@ import { Button } from "@/components/atoms/Button"; | |
import { CardAtom } from "@/components/atoms/CardAtom"; | ||
import { useNavigation } from "@/hooks/useNavigation"; | ||
import { LogData } from "@/types/logDisplayOptions"; | ||
import { ColumnContentTags } from "./components/ColumnContentTags"; | ||
import { ColumnContentTags } from "../components/ColumnContentTags"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why some of your imports are relative and others are absolute? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I took the habit of writing the imports that are linked to the same feature as relative, and the ones which are not as absolute. Is this bad practice ? |
||
|
||
export const LogDetailsTable = ({ logDetails }: { logDetails: LogData }) => { | ||
const cost_text: string = | ||
|
@@ -55,7 +55,10 @@ export const LogDetailsTable = ({ logDetails }: { logDetails: LogData }) => { | |
</tr> | ||
<tr> | ||
<td className="font-bold px-4 py-2">Tags</td> | ||
<ColumnContentTags tags={logDetails.tags} /> | ||
<ColumnContentTags | ||
tags={logDetails.tags} | ||
logId={logDetails.id} | ||
/> | ||
</tr> | ||
</tbody> | ||
</table> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this colourPicker specific to the button? If so, maybe include that in the name? Before I understood the function I thought if might be better living outside this folder, but actually it's looks specific to button? Probably good to make this clear in the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add it in the name, but this function is not exported so I thought it was clear that the scope was restrained to this file. I will rename it to buttonColourPicker.