Edit Links component #20

Merged
l42luo merged 0 commits from feat/linktree-editor-edit-links into main 2 years ago
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner
There is no content yet.
q63xu (Migrated from git.uwaterloo.ca) approved these changes 2 years ago
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed title from {-feat/linktree editor edit links-} to {+Edit Links component+}

changed title from **{-feat/linktree editor edit links-}** to **{+Edit Links component+}**
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

You would need to rebase this locally first @l42luo

You would need to rebase this locally first @l42luo
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 22 commits

  • 628153d2...ab544fc7 - 8 commits from branch main
  • 4279846c - janky static interface implemented
  • c8d8405d - react-beautiful-dnd not working but set up
  • b1590ee8 - drag and drop sometimes works
  • f3ef76e4 - drag and drop is fully functional
  • 7420392f - refactored EditLink to modify data within the component so that dragndrop still works
  • 54d81e06 - wip
  • aeb4891f - with steven's edits
  • 44ef918c - drag n drop simplified code
  • 87dff0b3 - drag n drop cleaned up and semi working lol fml
  • 30bf050c - drag n drop is refactored and officially fully functional
  • 87c7c249 - post verbal pr comments with Steven and Adi
  • 9f480959 - first pr
  • 10de1511 - removed unecessary type
  • 2ee1a39c - fixing merge conflicts

Compare with previous version

added 22 commits <ul><li>628153d2...ab544fc7 - 8 commits from branch <code>main</code></li><li>4279846c - janky static interface implemented</li><li>c8d8405d - react-beautiful-dnd not working but set up</li><li>b1590ee8 - drag and drop sometimes works</li><li>f3ef76e4 - drag and drop is fully functional</li><li>7420392f - refactored EditLink to modify data within the component so that dragndrop still works</li><li>54d81e06 - wip</li><li>aeb4891f - with steven&#39;s edits</li><li>44ef918c - drag n drop simplified code</li><li>87dff0b3 - drag n drop cleaned up and semi working lol fml</li><li>30bf050c - drag n drop is refactored and officially fully functional</li><li>87c7c249 - post verbal pr comments with Steven and Adi</li><li>9f480959 - first pr</li><li>10de1511 - removed unecessary type</li><li>2ee1a39c - fixing merge conflicts</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=51666&start_sha=628153d2b5ffe5c13e0b0ff4aec369643542c64e)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

marked this merge request as draft from 54d81e0660007b997cdb13f5c7aa376c0db83021

marked this merge request as **draft** from 54d81e0660007b997cdb13f5c7aa376c0db83021
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

I don't think we need a styled div over here. Can we replace <Container /> with a regular <div></div>?

I don't think we need a styled div over here. Can we replace `<Container />` with a regular `<div></div>`?
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

Wait, it's a draft, so I won't review right now :)

Wait, it's a draft, so I won't review right now :)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 3 of the diff

changed this line in [version 3 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=51673&start_sha=2ee1a39c3a8ea0acf4e62b217c5620e3d7975492#6f4be3437e185fee655e1ccc6e9906354c94ab33_7_5)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • be9a9f28 - drag and drop and responsive ui for adding a link functionalities

Compare with previous version

added 1 commit <ul><li>be9a9f28 - drag and drop and responsive ui for adding a link functionalities</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=51673&start_sha=2ee1a39c3a8ea0acf4e62b217c5620e3d7975492)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

marked this merge request as ready

marked this merge request as **ready**
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

nit: remove these console logs

nit: remove these console logs
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

nit: let's not set the name for a new link, keep it empty instead

nit: let's not set the name for a new link, keep it empty instead
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

nit: delete these lines

nit: delete these lines
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

nit: remove this function since we're not using it

nit: remove this function since we're not using it
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

replace with inline function from line 90 and use e.target.checked instead

replace with inline function from line 90 and use `e.target.checked` instead
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

replace with inline function from line 90

replace with inline function from line 90
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

nit: abstract this logic into a helper function and put this outside of the component body

nit: abstract this logic into a helper function and put this outside of the component body
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

I am using the name as the draggableId (it needs to be unique)

I am using the name as the draggableId (it needs to be unique)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#ce68616675aca98bae1ec0e1e41b640cb858881a_22_22)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#ce68616675aca98bae1ec0e1e41b640cb858881a_31_28)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#ce68616675aca98bae1ec0e1e41b640cb858881a_69_66)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#6f4be3437e185fee655e1ccc6e9906354c94ab33_22_32)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#6f4be3437e185fee655e1ccc6e9906354c94ab33_106_83)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#6f4be3437e185fee655e1ccc6e9906354c94ab33_117_94)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 4 of the diff

changed this line in [version 4 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275#6f4be3437e185fee655e1ccc6e9906354c94ab33_46_33)
l42luo commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 2 commits

  • 6c9aa560 - making changes according to pr comments
  • 91fb3afc - changes made for pr

Compare with previous version

added 2 commits <ul><li>6c9aa560 - making changes according to pr comments</li><li>91fb3afc - changes made for pr</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52284&start_sha=be9a9f28f2956d599f527a7bac1f7d702d13e275)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 6 commits

  • 91fb3afc...7389e555 - 4 commits from branch main
  • 6fcebab2 - Merge branch 'main' into feat/linktree-editor-edit-links
  • e06c7958 - Cleanup files

Compare with previous version

added 6 commits <ul><li>91fb3afc...7389e555 - 4 commits from branch <code>main</code></li><li>6fcebab2 - Merge branch &#39;main&#39; into feat/linktree-editor-edit-links</li><li>e06c7958 - Cleanup files</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52299&start_sha=91fb3afc1241ae9333ec1f97287639e1766520f9)
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

You should pull out these svgs into separate mini components so that we know what this svg is.

You should pull out these `svg`s into separate mini components so that we know what this `svg` is.
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

Generally, it's not good practice to name types this way. For eg:

  • class FooClass {} vs class Foo {}
  • const barNumber = 1 vs const bar = 1
Generally, it's not good practice to name types this way. For eg: - `class FooClass {}` vs `class Foo {}` - `const barNumber = 1` vs `const bar = 1`
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

In TypeScript, types that start with a T should be used when you're working with templates/generics.

In TypeScript, types that start with a `T` should be used when you're working with templates/generics.
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

Also, why do we have a separate types.ts file?

Also, why do we have a separate `types.ts` file?
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

You can inline the type:

export const useDragDrop: () => { displayDragDrop } = () => {
  ...
}

But this can look a bit weird 🤷

You can also export a function instead.

export function useDragDrop(): { displayDragDrop } {
  ...
}
You can inline the type: ``` export const useDragDrop: () => { displayDragDrop } = () => { ... } ``` But this can look a bit weird :shrug: You can also export a `function` instead. ``` export function useDragDrop(): { displayDragDrop } { ... } ```
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

Because TEditLink is being used in multiple files, I'll put it in utils/types for now and we can change it afterwards

Because `TEditLink` is being used in multiple files, I'll put it in `utils/types` for now and we can change it afterwards
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

hmmm I feel like both would add too much clutter, we should prob stick with declaring functions a certain way (in this case using arrow fns) and separating types instead of inlining it is better for readability imo

hmmm I feel like both would add too much clutter, we should prob stick with declaring functions a certain way (in this case using arrow fns) and separating types instead of inlining it is better for readability imo
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52325&start_sha=e06c795832799bc4e22595211ae7e24e2fdd62ae#acdb1a287fc8870013f5343e811aa40ed6aa2d4b_6_6)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 6 of the diff

changed this line in [version 6 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52325&start_sha=e06c795832799bc4e22595211ae7e24e2fdd62ae#2447f07e75e78e4030b251269cd0e63918e22a38_1_0)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • 4509a1c2 - Address PR comments

Compare with previous version

added 1 commit <ul><li>4509a1c2 - Address PR comments</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52325&start_sha=e06c795832799bc4e22595211ae7e24e2fdd62ae)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 3 commits

  • 006f12c6 - 1 commit from branch main
  • 570fb9b8 - Merge branch 'main' into feat/linktree-editor-edit-links
  • 15a046d4 - Cleanup merge changes

Compare with previous version

added 3 commits <ul><li>006f12c6 - 1 commit from branch <code>main</code></li><li>570fb9b8 - Merge branch &#39;main&#39; into feat/linktree-editor-edit-links</li><li>15a046d4 - Cleanup merge changes</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52330&start_sha=4509a1c28ea9c4e7f94d39907bcc5ccb74afa2a1)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 8 of the diff

changed this line in [version 8 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52573&start_sha=15a046d4a44d07bc59ac307d542429a108fcdc17#acdb1a287fc8870013f5343e811aa40ed6aa2d4b_48_56)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 2 commits

  • 2a8826df - Cleanup styles
  • 725dcb0a - Cleanup

Compare with previous version

added 2 commits <ul><li>2a8826df - Cleanup styles</li><li>725dcb0a - Cleanup</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52573&start_sha=15a046d4a44d07bc59ac307d542429a108fcdc17)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • e0ecec0e - Change EditLink naming

Compare with previous version

added 1 commit <ul><li>e0ecec0e - Change EditLink naming</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52574&start_sha=725dcb0a52517fd2c82d3fac0f0759bc00ddbea9)
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

no any pls :P

no any pls :P
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

actually we can fix this after we merge this PR

actually we can fix this after we merge this PR
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

Do we need a div? How about react fragments instead? <>...</>

Do we need a div? How about react fragments instead? `<>...</>`
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • fa28693b - Fix drag and drop

Compare with previous version

added 1 commit <ul><li>fa28693b - Fix drag and drop</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52577&start_sha=e0ecec0e30ba3755f9f869aafcb8526013e92e00)
a3thakra commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

What do you think about renaming LoginScreen to EditorPage and default export an unnamed function?

What do you think about renaming `LoginScreen` to `EditorPage` and default export an unnamed function?
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 11 of the diff

changed this line in [version 11 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52579&start_sha=fa28693b29d2fba6625189dc45328ab4b545400e#1300041c72ef072f4d429adf9886818d0ce15e24_64_64)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

changed this line in version 11 of the diff

changed this line in [version 11 of the diff](/csc/linktree/-/merge_requests/15/diffs?diff_id=52579&start_sha=fa28693b29d2fba6625189dc45328ab4b545400e#1300041c72ef072f4d429adf9886818d0ce15e24_73_73)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • 98708d18 - Address PR comments

Compare with previous version

added 1 commit <ul><li>98708d18 - Address PR comments</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52579&start_sha=fa28693b29d2fba6625189dc45328ab4b545400e)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

added 1 commit

  • 58b57e26 - Cleanup

Compare with previous version

added 1 commit <ul><li>58b57e26 - Cleanup</li></ul> [Compare with previous version](/csc/linktree/-/merge_requests/15/diffs?diff_id=52580&start_sha=98708d185ea531a93afc3cd704b66ea636b83089)
q63xu commented 2 years ago (Migrated from git.uwaterloo.ca)
Owner

approved this merge request

approved this merge request

Reviewers

The pull request has been merged.
Sign in to join this conversation.
No reviewers
No Label deployment
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: old/LinkList#20
Loading…
There is no content yet.