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

Remove AddThis integration #171

Merged
merged 3 commits into from
Jan 10, 2023
Merged

Conversation

johnhenley
Copy link
Collaborator

Remove AddThis integration

Changes made

  • Remove AddThis integration

PR Template Checklist

  • Fixes Bug
  • Feature solution
  • Other

Please mark which issue is solved

Close #167

@WillStrohl
Copy link
Member

Awesome PR, @johnhenley ! (as always...)

The only thing I see missing is the cleanup file and component update in the manifest file. Do you want me to add that for you later?

@johnhenley
Copy link
Collaborator Author

johnhenley commented Dec 7, 2022

Awesome PR, @johnhenley ! (as always...)

The only thing I see missing is the cleanup file and component update in the manifest file. Do you want me to add that for you later?

Hi @WillStrohl. Please add them. I didn't think a cleanup was needed since no files were removed?

Also this isn't meant to be a real release but I wanted to avoid the sqlprovider collisions that I created last time with multiple PRs using same version number. So I made this one 07.00.02. And I'm reserving 07.01.00 for the big kahuna PR that is coming for template changes #93. And any small intervening ones can be 07.0.xx.

@Timo-Breumelhof
Copy link
Contributor

Sounds good :-) But Shouldn't we go to V8 for the template changes?
As it's a breaking change?

@WillStrohl
Copy link
Member

Yes, ideally, breaking changes should not be in a minor or point release. So, should the next release be a major release?

@Timo-Breumelhof
Copy link
Contributor

When it contains the template change IMO it should be a major release.

@WillStrohl
Copy link
Member

I agree with semantic versioning... I'm simply asking if we should target a major release for our next release. 😁

@johnhenley
Copy link
Collaborator Author

When it contains the template change IMO it should be a major release.

Changing the storage location of templates might not be considered a breaking change. For new installations and upgrades, it's transparent but gives administrators option of editing templates by editing the files. Editing in the control panel still works. Same for moving themes out of desktopmodules. It's handled during installation and upgrades and doesn't require intervention. As long as it's well-tested and documented properly--that certain things have changed--like file names and locations. Just my two cents 😀

@johnhenley
Copy link
Collaborator Author

I agree with semantic versioning... I'm simply asking if we should target a major release for our next release. 😁

I'm undecided 😀. On the one hand if we do, we're burning thru version numbers which is not good. But if we don't, we might be breaking something that people are using. Example is addthis. We've announced its removal and no feedback. I don't consider that a breaking change. If we release it as a minor release and document it, isn't that good enough?

@Timo-Breumelhof
Copy link
Contributor

I agree it would be a bit fast on the other hand I so think it's a major change..

@WillStrohl
Copy link
Member

I don't think it's possible for us to run out of version numbers. :D Let's keep the current version and adjust later based on the updates we see queued up, ready to be tested for release. That should work for our process for now... Right?

@Timo-Breumelhof
Copy link
Contributor

Agreed, let's decide later

@johnhenley
Copy link
Collaborator Author

@WillStrohl can you approve this?

@johnhenley johnhenley added this to the 07.00.02 milestone Dec 28, 2022
@johnhenley johnhenley self-assigned this Dec 28, 2022
@WillStrohl WillStrohl added the enhancement New feature or request label Jan 10, 2023
@WillStrohl WillStrohl merged commit 1a698cc into DNNCommunity:dev Jan 10, 2023
@johnhenley johnhenley deleted the issues/167 branch February 5, 2023 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove AddThis
3 participants