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

OUT-1380 | Comment box designs #624

Open
wants to merge 6 commits into
base: feature/M11
Choose a base branch
from
Open

OUT-1380 | Comment box designs #624

wants to merge 6 commits into from

Conversation

arpandhakal
Copy link
Collaborator

@arpandhakal arpandhakal commented Feb 11, 2025

Changes

  • Added a mechanism to refresh srcs of images while on activity fetch.
  • Added styles and effects on comment input while a file is being dragged.
  • Added attachment icon alongside submit button in comment box with a new tapwrite update, version 1.1.83

Testing Criteria

@arpandhakal arpandhakal self-assigned this Feb 11, 2025
Copy link

linear bot commented Feb 11, 2025

Copy link

vercel bot commented Feb 11, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tasks-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 11, 2025 10:08am

arpandhakal and others added 6 commits February 11, 2025 15:46
- Added a mechanism to refresh srcs of images while on activity fetch.
- Added styles and effects on comment input while a file is being dragged.
- Added attachment icon alongside submit button in comment box with a new tapwrite update, version 1.1.83
@rrojan
Copy link
Collaborator

rrojan commented Feb 13, 2025

Please resolve conflicts here @arpandhakal

Comment on lines +83 to +90
const uploadFn = token
? async (file: File) => {
if (activeTask) {
const fileUrl = await uploadImageHandler(file, token ?? '', activeTask.workspaceId, task_id)
return fileUrl
}
}
: undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const uploadFn = token
? async (file: File) => {
if (activeTask) {
const fileUrl = await uploadImageHandler(file, token ?? '', activeTask.workspaceId, task_id)
return fileUrl
}
}
: undefined
const uploadFn = token && async (file: File) => {
if (activeTask) {
const fileUrl = await uploadImageHandler(file, token ?? '', activeTask.workspaceId, task_id)
return fileUrl
}
}

This should work if token is string | undefined

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 dont think this is possible because this is passed directly to tapwrite prop uploadFn which expects something like (file : File) => promise('string' | undefined') and not (" " | (file : File) => promise('string' | undefined'))

Comment on lines +102 to +109
dragCounter.current += 1
if (!isDragging) {
setIsDragging(true)
}
}

const handleDragLeave = () => {
dragCounter.current -= 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you please explain the need for dragCounter here... don't really catch the reason for it. Don't isDragging and dragCounter do the same thing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dragcounter is used because there could be multiple events firing while dragging, the events could be of same type or different type, for example drag enter, drag leave or other. To track the multiple events and stop flickering. The reason there are multiple events is because of nested dom structure. Lets say, there is a parent container and multiple child containers nested inside, if the drag is left from the innermost child container, then drag leave is fired causing setIsDragging to be false but since the drag is not out of the parent container, there is flickering.

@arpandhakal arpandhakal requested a review from rrojan February 14, 2025 09:10
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.

2 participants