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

Replace head_title with Kicker #140

Closed
wants to merge 4 commits into from
Closed

Replace head_title with Kicker #140

wants to merge 4 commits into from

Conversation

iFlameing
Copy link
Member

@davisagli I don't know how to create an alias for headtitle. can you please take care of it?

@mister-roboto
Copy link

@iFlameing thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@davisagli
Copy link
Member

@iFlameing I think we need to:

  1. Create IKicker like you did already
  2. Add IHeadTitle = IKicker in Python (so the old name still works if someone is importing it)
  3. Register 2 behaviors, the old one and the new one. I don't think there's an easy way to avoid this, since one behavior can only have one name.

@iFlameing
Copy link
Member Author

@davisagli I added the head title once again but I haven't tested it because I don't know how to test it. Also, it is a breaking change. where and how I should mention it?

@iFlameing iFlameing requested review from davisagli and tisto March 5, 2024 13:55
@davisagli
Copy link
Member

I added the head title once again but I haven't tested it because I don't know how to test it.

@iFlameing For starters, look at the result of the existing tests, which are failing. It looks like it is a problem to have 2 behavior registrations for the same interface, and here it is the same object even though it has an alias. To fix this, define IHeadTitle as a subclass of IKicker:

class IHeadTitle(IKicker):
    """alias for backwards-compatibility"""

Next, test this change with our customer project. You can pip install this branch from github (https://pip.pypa.io/en/stable/topics/vcs-support/). If our backwards-compatibility efforts are successful, everything in that project should still work even if you don't change the content types to use the new behavior.

Also, it is a breaking change. where and how I should mention it?

  1. Update the readme where it discusses the head title behavior
  2. Mention the new title of the field in the changelog. I think we decided we can call this one a bugfix.

@tisto
Copy link
Member

tisto commented Apr 16, 2024

@iFlameing do you have time to look into this during the week? I would love to start using this for our different Plone distribution projects.

src/plone/volto/behaviors/configure.zcml Show resolved Hide resolved
src/plone/volto/behaviors/configure.zcml Show resolved Hide resolved
src/plone/volto/behaviors/kicker.py Show resolved Hide resolved
@@ -38,8 +38,8 @@ msgid "Creates a default page for the site using slate blocks"
msgstr ""

#: plone/volto/behaviors/configure.zcml:42
msgid "Head title field"
msgstr "Kopftitel Feld"
Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep the existing translations for "head title"; it's better than removing the translations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that it is automatically updated once we removed the text. Translation is automatically deleted.

@iRohitSingh
Copy link
Contributor

@davisagli @tisto I have created a new pr in favour of this #163

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.

5 participants