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

fixes #142 (App.jsx, BgColorSidePanel.jsx, canvas.js): The line draw… #263

Merged

Conversation

mondalsurojit
Copy link
Contributor

fixes #142 (App.jsx, BgColorSidePanel.jsx, canvas.js): The line drawn gets un centered from the cursor when drawing further downwards on the canvas

Type of change ☑️

  • Bug fix (non-breaking change which fixes an issue)
  • Code style update (formatting, local variables)

Checklist: ☑️

  • My code follows the guidelines of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly wherever it was hard to understand.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added things that prove my fix is effective or that my feature works.
  • Any dependent changes have been merged and published in downstream modules.

How Has This Been Tested? ⚙️

Draw.it.out.-.Google.Chrome.2024-06-02.17-08-14.mp4

…js): The line drawn gets un centered from the cursor when drawing further downwards on the canvas
Copy link

vercel bot commented Jun 2, 2024

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

Name Status Preview Comments Updated (UTC)
draw-it-out ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 29, 2024 7:15am
draw-it-out-zbd1 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 29, 2024 7:15am

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome to Our repository.🎊 Thank you so much for taking the time to point this out.

@mondalsurojit
Copy link
Contributor Author

@singodiyashubham87 Sir I request you to review my PR...and I hope you will consider this as a level 3 issue...as this issue remained unsolved for a long time and it also took around 2 days just to figure out where the problem actually is...but finally I solved it...thank you in advance...

@singodiyashubham87
Copy link
Owner

@0xabdulkhalid Kindly review this PR & suggest your feedback.

src/App.jsx Outdated Show resolved Hide resolved
@0xabdulkhaliq
Copy link
Collaborator

Note

BTW, I mistakenly approved changes. I was about to request changes!

@mondalsurojit
Copy link
Contributor Author

@0xabdulkhalid I removed the extra conditional class...I hope now its okay...please see once...
@singodiyashubham87

src/App.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@0xabdulkhaliq 0xabdulkhaliq left a comment

Choose a reason for hiding this comment

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

Good to go 🚀

@singodiyashubham87, now this PR is ready to be merged.

@singodiyashubham87
Copy link
Owner

@mondalsurojit Resolve the conflicts.

@mondalsurojit
Copy link
Contributor Author

@singodiyashubham87 I have resolved the merge conflicts...now this branch is ready to be merged...

@singodiyashubham87
Copy link
Owner

@mondalsurojit Merge the main branch & push the changes.

Your PR:

image

Actual

image

@singodiyashubham87
Copy link
Owner

singodiyashubham87 commented Jun 9, 2024

@mondalsurojit While resolving the conflicts you have removed the code that was added by other PRs to solve other issues, which is not the correct way to resolve the conflicts, you have to modify your code according to the main branch & not remove the main branch's code to make your's one working.

There are many functionalities broken.

  1. the tour steps are missing, there is only one tour step which tells about the color palette & then directly tells to start drawing
  2. After selecting shapes, when we again select the pencil tool, it defaults it to solid brush & when we try to change to dotted or dashed, it does not change.
  3. The cursor on canvas is now pointer & not crosshair.
  4. Improper UI, the cross button is overlapping the canvas & there is very less space between the canvas & tools container
    image

@mondalsurojit
Copy link
Contributor Author

mondalsurojit commented Jun 9, 2024

@singodiyashubham87 Hello!
I had actually checked everything before creating a PR, and everything was fine...Yes 2 bugs were noticed by me, which was caused due to some other previous PRs, and I believe those are directly or indirectly not connected to the code , I have written...

This is what my branch currently looks on my PC...
https://github.com/singodiyashubham87/Draw-it-out/assets/114282267/edd4072d-0e6d-4815-9a2c-28eb842f0c73

If you have seen my video, then you can see:

  1. There are yet 3 steps, properly available one after the other in order. (p.s. still 1 step is missing, but I can't help, as that is caused due to some other problem)
  2. (After selecting shapes, when we again select the pencil tool, it defaults it to solid brush & when we try to change to dotted or dashed, it does not change.) I have also demostrated this in the video.
  3. The cursor on canvas is still a crosshair
    4)The cross button is actually not overlapping...

I don't know, why you are facing such errors, on your side, it was not supposed to happen, as I have checked everything thoroughly before doing a PR (keeping aside 2 other bugs)....

Please check once, if everything is okay on your side

@mondalsurojit
Copy link
Contributor Author

@singodiyashubham87 What I feel is that, when I am making a branch from "main" and someone else is also making a branch form "main"...Both of us are simultaneously working on an issue, but if I create PR before him, and then he tries to create PR, the code will break or create merge conflicts, and this is unavoidable...somehow the whole scenario looks like a race condition...
For the proper merging, such that there is minimum to no merge conflicts, the whole workflow should be serialized...only one person working at a time on a PR, and they should have a deadline....

@singodiyashubham87
Copy link
Owner

@mondalsurojit Maybe the UI change is because of OS resolution, You are working on Windows & I'm working on Linux.
I would recommend you to copy the changes that you made & create a new clone of the project in your local machine then paste the changes & create a new fresh PR, which won't have any conflicts with the merged PRs.

@singodiyashubham87
Copy link
Owner

@mondalsurojit working on this?

@singodiyashubham87
Copy link
Owner

@mondalsurojit Conflicts, I recommend you to create a new fork.

@mondalsurojit
Copy link
Contributor Author

@singodiyashubham87 I have resolved it, actually very minute conflicts, like a portion I had deleted, or a function's name that I had changed....Please see once

@singodiyashubham87
Copy link
Owner

@mondalsurojit I've reviewed your PR & here are some changes that you need to make:

  1. The cursor is a pointer in the latest changes but your still has the crosshair
  2. The gap between canvas & tools is not appropriate

Actual:

image

Yours one:

image
3. In dark mode, the guidelines part looks different

Actual:

image

Yours:

image

@singodiyashubham87
Copy link
Owner

@mondalsurojit Well Done, Keep contributing :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] The line drawn gets un centered from the cursor when drawing further downwards on the canvas.
3 participants