Feat/add tag binding #90

Lerri-Cofannos · 2023-10-06T14:31:28Z

Waiting for TagsAPI routes to be functional.
Ticket for logs table
Ticket for details page

clairesquires · 2023-10-10T14:40:18Z

front/src/components/atoms/Button.tsx

+
+const colourPicker = (colour: string): string => {
+    const blueStyle = "bg-theodo-turquoise hover:bg-omnilog-clear-blue";


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

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.

clairesquires · 2023-10-10T14:42:25Z

front/src/components/atoms/ButtonIcon.tsx

Is there any reason that this component can't extend your non-icon button?

The style of both components are a bit different: this one has grey scale colours and is a circle.

clairesquires · 2023-10-10T14:58:20Z

front/src/features/tables/components/TagPopup.tsx

+                    <Button colour="blue" onClick={() => {}}>
+                        Useless Validation
+                    </Button>


What is this?

It was to prepare for the "Create New Tag" button, but since it would be pushed to prod before this feature appears, then I'll remove it. I'll also remove the "Cancel" button which is redundant with the top exit button of the Popup component

clairesquires · 2023-10-10T15:30:22Z

front/src/features/tables/views/LogDetailsTable.tsx

@@ -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";


Curious why some of your imports are relative and others are absolute?

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 ?

CallumHemsley · 2023-10-11T09:17:37Z

front/src/components/atoms/ButtonIcon.tsx

+import { MouseEventHandler } from "react";
+
+type ButtonIconProps = {
+    onClick: MouseEventHandler<HTMLButtonElement> | (() => void);


Why do you need two types here? Why not just mouse event handler?

I thought that the two were too different, but you are right, casting () => void to the event handler works great!

front/src/components/atoms/ButtonIcon.tsx

CallumHemsley · 2023-10-11T09:19:44Z

front/src/components/atoms/Popup.tsx

+export const Popup = ({ children, open, setOpen }: PopupProps) => {
+    return (
+        <Transition.Root show={open} as={Fragment}>
+            <Dialog as="div" className="relative z-10" onClose={() => {}}>


close is a different ticket im assuming?

Close is a compulsory prop of the Dialog component. It is called when clicking outside of the modal. I deactivated that behaviour to avoid a bug. The bug was that exiting with such a click navigated the user to the corresponding log. I suspect that the event that I had caught with e.stopPropagate in openPopup is freed and thus a click is made on the log's row, making the user navigate to it. I did not find a quick solution on the documentation.

How do users close the dialog in this case?

There is a button in the top right corner (see screenshot)

CallumHemsley · 2023-10-11T09:22:40Z

front/src/features/tables/components/ColumnContentTags.tsx

+    logId: string;
+}) => {
+    const [tagArray, setTagArray] = useState<Tag[]>(tags);
+    const addTag = (tag: Tag) => {


so this is just setting it locally for now?

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.

CallumHemsley · 2023-10-11T09:23:38Z

front/src/features/tables/components/TagPopup.tsx

+};
+
+export const TagPopup = ({ logId, addTagToDisplay }: TagPopupProps) => {
+    const { isPopupOpen, openPopup, setIsPopupOpen, tags, selectTag } =


this seems odd. Wouldn't setIsPopUpOpen be dependent on the openPopup call? I don't think we need this much state

maybe it's needed but this seems a bit confusing to me

The openPopup is dependent on setIsOpenPopup, but adds an event handler to prevent click propagation. I have replaced the function setIsOpenPopup with closePopup to stay consistent in the naming

CallumHemsley · 2023-10-11T09:25:35Z

front/src/features/tables/helpers/tagArrayManagement.ts

+    });
+};
+
+const compareStrings = (stringA: string, stringB: string): number => {


hmm I thought sort basically did this by default, interesting you needed to define a sort function yourself since its a basic string sort

Can't you just do return newTagArray.sort(tagA.toLowerCase(), tagB.toLowerCase());

I did a little research and found a built-in function that compares string based on alphabetical order. New commit incoming!

CallumHemsley · 2023-10-11T09:27:36Z

front/src/features/tables/hooks/useTagPopup.ts

+        setIsPopupOpen(true);
+    };
+
+    return { isPopupOpen, openPopup, setIsPopupOpen, tags, selectTag };


yeah I feel like you shouldn't need to expose setIsPopupOpen outside fo this hook, like an openPopUp and closePopUp method shoudl only be exposed

Good catch! I'll add a commit for this.

Lerri-Cofannos force-pushed the feat/add-tag-binding branch from 461452f to 06986a5 Compare October 6, 2023 16:04

Lerri-Cofannos added 10 commits October 10, 2023 11:50

feat(front): create calls from services to tag api

b3e7ca6

feat(front): add red option in button atom

e46f575

feat(front): create button atom with icon

29d497b

feat(front): create popup atom

bb7717c

feat(front): safe cast response from tagsAPI

1d49193

feat(front): create tag popup

bbdd338

refactor(front): move tables to views subdirectory

d09e3a9

fix(front): adjust getTags to server response

f2a7b30

style(front): improve popup UI

20a1eff

refactor(front): move LogsData to prisma folder

9bcb131

Lerri-Cofannos force-pushed the feat/add-tag-binding branch from d98311e to 9bcb131 Compare October 10, 2023 11:14

Lerri-Cofannos added 3 commits October 10, 2023 12:23

fix(front): update path to LogsData in page test

2b55f4a

style(front): change ButtonIcon outline colour

e19a887

feat(front): improve UX speed when adding tags

0b3c3c6

Lerri-Cofannos marked this pull request as ready for review October 10, 2023 14:14

refactor(front): move the propagation prevention to useTagPopup

b9af47d

clairesquires reviewed Oct 10, 2023

View reviewed changes

refactor(front): move connectTagToLog to relevant file

5e4824f

clairesquires reviewed Oct 10, 2023

View reviewed changes

Lerri-Cofannos added 5 commits October 10, 2023 17:11

refactor(front): group tag helper functions

72f975e

feat(front): handle errors while connecting log and tag

c64b9ee

fix(front): prevent adding existing tag to array

39034c6

refactor(front): rename colour picker function

4a22a4e

fix(front): remove unused buttons on popup

e800c1b

CallumHemsley reviewed Oct 11, 2023

View reviewed changes

front/src/components/atoms/ButtonIcon.tsx Outdated Show resolved Hide resolved

CallumHemsley reviewed Oct 11, 2023

View reviewed changes

CallumHemsley approved these changes Oct 11, 2023

View reviewed changes

Lerri-Cofannos added 4 commits October 11, 2023 10:40

refactor(front): create closePopup func in tag hook

badbd1d

refactor(front): use built-in string comparison

56006ed

refactor(front): clean className string format

dec6e0e

refactor(front): remove unused type option for button

8de6f1a

GuillaumeBarra approved these changes Oct 11, 2023

View reviewed changes

Lerri-Cofannos merged commit 15227d0 into main Oct 11, 2023
1 check passed

Lerri-Cofannos deleted the feat/add-tag-binding branch October 11, 2023 10:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/add tag binding #90

Feat/add tag binding #90

Lerri-Cofannos commented Oct 6, 2023 •

edited

Loading

clairesquires Oct 10, 2023

Lerri-Cofannos Oct 10, 2023

clairesquires Oct 10, 2023

Lerri-Cofannos Oct 10, 2023

clairesquires Oct 10, 2023

Lerri-Cofannos Oct 10, 2023

clairesquires Oct 10, 2023

Lerri-Cofannos Oct 10, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

GuillaumeBarra Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

CallumHemsley Oct 11, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

CallumHemsley Oct 11, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023

CallumHemsley Oct 11, 2023

Lerri-Cofannos Oct 11, 2023


		const colourPicker = (colour: string): string => {
		const blueStyle = "bg-theodo-turquoise hover:bg-omnilog-clear-blue";

Feat/add tag binding #90

Feat/add tag binding #90

Conversation

Lerri-Cofannos commented Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lerri-Cofannos commented Oct 6, 2023 •

edited

Loading