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

Added clear button and refractored code #19

Merged
merged 2 commits into from
Nov 23, 2024

Conversation

ravjot07
Copy link
Contributor

Added clear button that Clears the canvas and reset the drawing lines. Contribution towards #14

Clear_Button.mp4

@ravjot07
Copy link
Contributor Author

Hey, @quozl @chimosky
Can you review this PR i have added a clear button and refactor the code for better understanding

@ravjot07
Copy link
Contributor Author

@quozl can you review this pr

js/activity.js Outdated Show resolved Hide resolved
css/activity.css Show resolved Hide resolved
Comment on lines 4 to 6
require("easel");
require("handlebars");
var shapes = require("activity/shapes");
Copy link
Member

Choose a reason for hiding this comment

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

Are these used here?

js/activity-works.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
js/activity.js Outdated Show resolved Hide resolved
@chimosky
Copy link
Member

See #15 to make sure you've addressed the concerns listed there, reviewed, not tested.

The code refactoring shouldn't be in the same commit as the clear button change, keep both separate as it's easier to review that way.

@ravjot07
Copy link
Contributor Author

See #15 to make sure you've addressed the concerns listed there, reviewed, not tested.

The code refactoring shouldn't be in the same commit as the clear button change, keep both separate as it's easier to review that way.

yes i have addressed the concerns listed in #15.... mainly they discussed about position of the button and its icon visibility and its functioning about deleting only the drawing rather than whole canvas...... i have covered all of that

@ravjot07
Copy link
Contributor Author

@chimosky are there any more changes or suggestion or we should merge this PR

@chimosky
Copy link
Member

chimosky commented Nov 22, 2024

Would be great to have better commit messages than "Minor Updates", see making commits.

I'll test this once I can.

@ravjot07
Copy link
Contributor Author

Would be great to have better commit messages than "Minor Updates", see making commits.

I'll test this once I can.

@chimosky sure i will take care of commit msg next time.

@quozl
Copy link
Contributor

quozl commented Nov 23, 2024

You can and should change commit messages now rather than wait for next time.

@ravjot07
Copy link
Contributor Author

You can and should change commit messages now rather than wait for next time.

Done @quozl

@quozl
Copy link
Contributor

quozl commented Nov 23, 2024

Tested. Reviewed.

@quozl quozl merged commit d37e35b into sugarlabs:master Nov 23, 2024
ravjot07 added a commit to ravjot07/connect-the-dots-activity that referenced this pull request Nov 24, 2024
@ravjot07 ravjot07 mentioned this pull request Dec 8, 2024
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.

3 participants