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

Implemented delete button #90

Merged
merged 24 commits into from Nov 9, 2022
Merged

Implemented delete button #90

merged 24 commits into from Nov 9, 2022

Conversation

ghost
Copy link

@ghost ghost commented Oct 9, 2022

I have implemented the delete button

Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. PTAL CI failure.

Additionally, changes should be backwards compatible to avoid surprising existing users with a UI change when they update; so I suggest adding a config option that turns on the delete buttons instead of having them enabled by default.

Lastly, could you also add a nightwatch test for the new functionality. Thank you!

@ghost
Copy link
Author

ghost commented Oct 12, 2022

Okay, Clemens, I will manage to get that done by the next weekend

- config option displayDeleteButtonOnHistory to display the delete button
- Implemented test in NightWatch.js
- Implemented examples in javascript and React.js
@ghost
Copy link
Author

ghost commented Oct 15, 2022

I have implemented everything that you required me.
Now it can be set to see the button option through the option displayDeleteButtonOnHistory in the options object.
I also have implemented the tests in NightWatch.js

@ghost
Copy link
Author

ghost commented Oct 19, 2022

Clemens, hello... Have you teted out the delete button?
Is everything ok with it?

mathquill4quill.css Outdated Show resolved Hide resolved
- changed the width back the default CSS class style
- added an inline width for the container div when using the delete button
@ghost
Copy link
Author

ghost commented Oct 20, 2022

Clemens, I solved this issue by adding an inline style for the container div when using the deleteButton, ok?
In the line 341 of the mathquill4quill.js file
container.style.width = "318px";

@ghost
Copy link
Author

ghost commented Oct 25, 2022

Is everythin correct now, @c-w?

@c-w
Copy link
Owner

c-w commented Oct 27, 2022

Apologies, I haven't gotten around to re-review this yet. I'll take a look ASAP.

@ghost ghost requested a review from c-w November 3, 2022 14:00
Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

Apologies for the delay in reviewing this. To speed up a merge, I took the liberty to make a few changes. Take a look at my patch with the suggested diff and git apply it if you're satisfied with the fixes.

@ghost
Copy link
Author

ghost commented Nov 5, 2022

Hey, Clemens, I applied most of those changes... Expect for the index
We actually have to look for each element we want to delete with const index = history.indexOf(element), it doesn't work through the index element of the array as you can see in the video attached down bellow

mathquill4quill.Demo.-.Google.Chrome.2022-11-05.13-27-08.mp4

Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. PTAL CI failure (you can also run it locally with npm run lint, probably a good idea as for some reason Github asks me to continuously re-approve the CI run) and the nitpicks regarding keeping the git history clean. Thank you.

mathquill4quill.css Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
lucasbbsdk and others added 5 commits November 8, 2022 20:18
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
@ghost ghost requested a review from c-w November 8, 2022 23:24
Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

More issues from CI. I think I listed them all, but to be sure do run npm run lint locally after making the changes so that we can get this merged in the next round. Alternatively you can also allow edits by maintainers on the PR so that I can make these nitty changes directly and get it released. Thank you.

mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
mathquill4quill.js Outdated Show resolved Hide resolved
tests/deleteButton.test.js Outdated Show resolved Hide resolved
tests/deleteButton.test.js Outdated Show resolved Hide resolved
tests/deleteButton.test.js Outdated Show resolved Hide resolved
tests/deleteButton.test.js Outdated Show resolved Hide resolved
tests/deleteButton.test.js Outdated Show resolved Hide resolved
lucasbbsdk and others added 6 commits November 9, 2022 10:14
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
lucasbbsdk and others added 3 commits November 9, 2022 10:16
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
Co-authored-by: Clemens Wolff <clemens@justamouse.com>
@ghost
Copy link
Author

ghost commented Nov 9, 2022

More issues from CI. I think I listed them all, but to be sure do run npm run lint locally after making the changes so that we can get this merged in the next round. Alternatively you can also allow edits by maintainers on the PR so that I can make these nitty changes directly and get it released. Thank you.

I have given you the right to edit my pull request, @c-w

Copy link
Owner

@c-w c-w left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@c-w c-w merged commit e5c000d into c-w:master Nov 9, 2022
@c-w
Copy link
Owner

c-w commented Nov 9, 2022

This has been released in v2.4.0

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.

None yet

2 participants