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

Feat: Add Undo and Redo functionality #26

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ravjot07
Copy link
Contributor

@ravjot07 ravjot07 commented Dec 8, 2024

Add: Undo and Redo for pen stoke..

Undo-Redo.mp4

@ravjot07 ravjot07 mentioned this pull request Dec 8, 2024
@quozl
Copy link
Contributor

quozl commented Dec 8, 2024

Thanks.

  • your commit messages do not follow our guidance, in particular
    • "added" should be "add",
    • the first lines are too long,
    • there's nothing but first lines,
    • "implemented" is vague,
    • typo "stoke",
  • you have missing newline on files,
  • the undo and redo icons do not follow Sugar style, please use our existing icons for undo and redo; remember each activity is just one of several, so don't focus only on this repository.

@walterbender, please review? (You posted on issue #14).

@ravjot07
Copy link
Contributor Author

ravjot07 commented Dec 10, 2024

Thanks.

  • your commit messages do not follow our guidance, in particular

    • "added" should be "add",
    • the first lines are too long,
    • there's nothing but first lines,
    • "implemented" is vague,
    • typo "stoke",
  • you have missing newline on files,

  • the undo and redo icons do not follow Sugar style, please use our existing icons for undo and redo; remember each activity is just one of several, so don't focus only on this repository.

@walterbender, please review? (You posted on issue #14).

@quozl can you review this undo and redo button styles and i have resolved most of the suggestions as you mentioned sir

@quozl
Copy link
Contributor

quozl commented Dec 12, 2024

You can find edit-undo.svg in sugar-artwork repository, icons/scalable/actions

Signed-off-by: Ravjot Singh <[email protected]>
@ravjot07
Copy link
Contributor Author

sugar-artwork

@quozl Undo and Redo button Updated according to sugar-artwork

@quozl
Copy link
Contributor

quozl commented Dec 16, 2024

Thanks. @chimosky, @walterbender any review comments?

css/activity.css Outdated Show resolved Hide resolved
icons/redo-button.svg Outdated Show resolved Hide resolved
Copy link
Member

@chimosky chimosky left a comment

Choose a reason for hiding this comment

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

Your commit message says "Implemented a stoke history data structure", I'm guessing you meant "stroke history".

If that's the case then you should edit the commit message.

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

chimosky commented Dec 17, 2024

Reviewed, not tested.

I'm genuinely curios why you add lots of spaces in most of your commits, a commit message says one thing and after looking at the commit most of the changes are new line additions which is just noise.

@ravjot07
Copy link
Contributor Author

Reviewed, not tested.

I'm genuinely curios why you add lots of spaces in most of your commits, a commit message says one thing and after looking at the commit most of the changes are new line additions which is just noise.

@chimosky This could be due to the linting tools configured in my setup, which might be automatically adjusting whitespace or adding new lines. I’ll review my configuration to ensure cleaner commits going forward.

@ravjot07
Copy link
Contributor Author

@quozl @chimosky

@ravjot07
Copy link
Contributor Author

@quozl if there are no further changes we should merge this pr

@quozl
Copy link
Contributor

quozl commented Dec 23, 2024

#26 (comment) is my review of commit messages, no changes have happened yet.

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