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

New features and improvements #1419

Merged
merged 16 commits into from
Oct 17, 2023
Merged

New features and improvements #1419

merged 16 commits into from
Oct 17, 2023

Conversation

EchoEllet
Copy link
Collaborator

@EchoEllet EchoEllet commented Oct 5, 2023

Features:

  1. The desktop context menu that shows when you click on the image was quite buggy on macOS, so I added an option to the developer to force the use of the mobile context menu but the resizing feature will not be available only on desktop, I excluded it as option
  2. Add the QuillImageUtilities class which has helpful functions for developers, you could remove this if you want to

Improvements:

  • The way to check if the video URL is from YouTube or not is now handled in a better way
  • Better documentations
  • Important: The default action when removing the image from the editor is to delete the image if it exists, however on desktop we don't want to touch user files since the way to handle files on desktop is different from mobile (iOS, Android) so I handle this as well, otherwise, users will get a crash when we remove the image øn macOS, on other desktop platforms the app will delete the user image which something we should never do as developers, the trust is important as humans not just between developers and users
  • the isImageBase64() has been improved
  • Fix issue #1418
  • Update the minimum version of device_info_plus to 10.0.0 of flutter_quill

And I don't know where there is a conflict between all the branches and some changes I already made before
for that conflict, I haven't slept in a while so I'm not focused, I could make more mistakes

And speaking of mistakes, the library should be tested on Windows and Linux as well

I have noticed some other bugs in the editor on macOS that are not related to the changes I made

Also on the web, there is a small issue I will leave it for someone else for now, but if no one migrates the code then I will have to do it myself

Web issue

and if you use flutter_inappwebview please update it to the minimum version since there was a bug that caused the app to not compile and install the pods in ios using CocoaPods, it has been fixed in the latest version

Edit: I think I have made the task for you guys more difficult since I didn't fetch the changes made by you and the others and then try to make the new changes, I fixed the conflicts however It requires more review from you

The branch is outdated
My previous pull request

… delete unused dependencies and upgrade all packages and plugins and remove gallery_saver which has not been updated for more than 23 months, it was a great plugin but it old now, and I also add some simple documentation and other minor improvements
…new event to allow the user to confirm removing the image before actually remove it, translated some text in Arabic languague since it was incorrect or missing
Copy link
Owner

@singerdmx singerdmx left a comment

Choose a reason for hiding this comment

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

@Jon-Salmon please have a look

@singerdmx
Copy link
Owner

@Jon-Salmon ping

Copy link
Collaborator

@Jon-Salmon Jon-Salmon left a comment

Choose a reason for hiding this comment

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

In general I'm happy with what you're trying to add, but a lot of this code is quite mangled up from what looks like being based off an old branch. I've tried to highlight where these issues are, but you are probably best rebasing off the current main.

Also, please update the changelog file

@EchoEllet EchoEllet requested a review from Jon-Salmon October 11, 2023 13:51
@EchoEllet
Copy link
Collaborator Author

I have fixed almost all the issues, as I mentioned in the previous pull request my sleep is not regular
and this is why I made too many mistakes in the new pull request

Thank you for your time and efforts.

CHANGELOG.md Outdated Show resolved Hide resolved
@singerdmx
Copy link
Owner

@Jon-Salmon ping

@Jon-Salmon
Copy link
Collaborator

Looks pretty good! One final change from my perspective and then LGTM

@singerdmx
Copy link
Owner

@freshtechtips please make the final change and let's merge it

@singerdmx
Copy link
Owner

@freshtechtips can you resolve the conflict and I will merge it

@singerdmx
Copy link
Owner

The conflict is from change log and version

@EchoEllet
Copy link
Collaborator Author

I fixed the conflicts and pushed the changes however it still shows me the message "This branch has conflicts that must be resolved" Do I need to do this in GitHub?? and then fetch the changes to merge them with my local one?? I want my local repository to be up to date because I plan on sending more pull requests for further improvements, that is if you want to or don't mind

@EchoEllet
Copy link
Collaborator Author

I did it from remote one on github since I will rebase anyway

@singerdmx singerdmx merged commit 5d30113 into singerdmx:master Oct 17, 2023
1 check passed
@singerdmx
Copy link
Owner

@Jon-Salmon you can make additional changes per your preference

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