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

Resolve #4556 - Fix: add protocol automatically #4566

Merged
merged 4 commits into from
Dec 20, 2023
Merged

Resolve #4556 - Fix: add protocol automatically #4566

merged 4 commits into from
Dec 20, 2023

Conversation

daiki0381
Copy link
Contributor

@daiki0381 daiki0381 commented Dec 20, 2023

What does this PR do?

  • Remove 'Use Default Protocol'
  • Check protocol when initializing the dialog if linkInfo.url is defined
  • Check protocol during onChange event if linkInfo.url is undefined

What are the relevant tickets?

#4556

Video (if for frontend)

http protocol
2023-12-20.13.31.35.mov
2023-12-20.13.28.04.mov
2023-12-20.13.27.07.mov
2023-12-20.13.26.07.mov
mailto protocol
2023-12-20.13.31.03.mov
tel protocol
2023-12-20.13.30.18.mov

Checklist

  • Remove 'Use Default Protocol'
  • Check protocol when initializing the dialog if linkInfo.url is defined
  • Check protocol during onChange event if linkInfo.url is undefined
  • Add test case

@daiki0381 daiki0381 marked this pull request as ready for review December 20, 2023 04:37
@daiki0381
Copy link
Contributor Author

@diemendesign PR created. Please review this PR at your convenience.

@DennisSuitters
Copy link
Member

I'll also add your changes to the Skunkworks repository, if all good, then I'll merge.

DennisSuitters pushed a commit to summernote/skunkworks that referenced this pull request Dec 20, 2023
@DennisSuitters DennisSuitters merged commit c96ca1d into summernote:develop Dec 20, 2023
2 checks passed
@DennisSuitters
Copy link
Member

Thanks for your contribution, looks good, I've also added it to the Skunkworks repository.

@daiki0381
Copy link
Contributor Author

@diemendesign Thank you for the review! How should I apply these revisions to my project? (It seems they are not yet reflected on summernote.org.)

@DennisSuitters
Copy link
Member

No, they won't be, you'll need to build the project, which I'm guessing you would have to test your changes before submitting your PR's?

@daiki0381
Copy link
Contributor Author

@diemendesign I apologize for any confusion. There might be a misunderstanding. The changes work well in my local environment. However, for my project, I have installed Summernote via npm, but the latest version on npm is only updated until 2021. To incorporate these changes, should I clone the repository, rebuild it (updating the dist directory), and then publish it as an npm package before integrating it into my project? If there are any alternative methods, I would be grateful if you could inform me.

@DennisSuitters
Copy link
Member

Ah, sorry, I had a feeling that may have been why you were asking.
You could clone it, and deploy your own npm to include Summernote. I don't use it that way myself.

Also, out of curiosity, can you try something for me please? I;m not sure if something is interfering with this, but when adding a link the first time, the url (or href) isn't added to the editing area, but the element is, along with the "Text to Display". If I then edit the link via the popup, add the URL, it's then added. I just wanted to check with you, that a bug hasn't crept in, or if there's something interfering my end. I should mention though, that the Skunkworks version that I'm using, also has an option for adding a list of links that can be selected from a dropdown, that populates the other fields. I've tried disabling that, and the issue still persists.

@DennisSuitters
Copy link
Member

Upon further inspection, it's not the list links option interfering with the link (or rather the adding of the href) that's the issue, something else is going on. Requires more investigation, sigh 1 step forward, 5 steps back, lol. Oh, I also checked if it was because testing locally I was using http rather than https, FYI, it makes no difference.

@daiki0381
Copy link
Contributor Author

daiki0381 commented Jan 5, 2024

@diemendesign I've applied this patch in my local environment and project, and it appears to function correctly. Would you confirm if there are any issues when this is tested with the Summernote repository? There might be some interference specifically with the Skunkworks version.

@DennisSuitters
Copy link
Member

In the Skunkworks repo I added this summernote/skunkworks@59ff12e#diff-99e04083a94e9e7915aa04146bce214b2b8d7f47af05ed42000aff010f1ed3a7R161 to get it to work.

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