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

Refactor publish view controls and content table to viewlets #133

Merged
merged 10 commits into from
Feb 8, 2023

Conversation

ramonski
Copy link
Contributor

@ramonski ramonski commented Feb 7, 2023

Description of the issue/feature this PR addresses

This PR refactors the setup-button, language selector and content listing of the publish view into separate viewlets.

Current behavior before PR

All elements inside the publish view template

Desired behavior after PR is merged

Elements are rendered by viewlets

--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.

@ramonski ramonski changed the title Refactored publish view controls and content table to viewlets Refactor publish view controls and content table to viewlets Feb 7, 2023
Copy link
Member

@xispa xispa left a comment

Choose a reason for hiding this comment

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

I was not able to do functional testing. Did you missed to add the module senaite.impress.browser.publish maybe?



class PublishContentViewlet(ViewletBase):
"""Viewlet that is displayed when the Auditlog is disabled
Copy link
Member

Choose a reason for hiding this comment

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

Should be something like "Viewlet that is displayed in the publish view above the publish controls"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies, I splitted the PR in 2 parts and forgot to remove some parts that should come next (replacement of the listing table to use senaite.app.listing). Removed in 3183a55

I also fixed the docstring in 99f1d6d

manager=".interfaces.IPublishHtmlHeadViewlets"
class=".resources.ResourcesViewlet"
permission="zope2.View"
layer="senaite.impress.interfaces.ILayer"
Copy link
Member

Choose a reason for hiding this comment

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

Why you rely on ILayer instead of ISenaiteImpressLayer? Also, why to not remove the former and leave only the latter in interfaces.py?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I changed that in e109cb0

<td tal:define="state model/review_state|nothing">
<span tal:condition="state"
i18n:translate=""
i18n:domain="bika"
Copy link
Member

Choose a reason for hiding this comment

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

These i18n directives can be removed, span contents won't be translated

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, also the domain was wrong.
I removed it in 8fcbd51

This template will be replaced with a content table of senaite.app.listing in the next PR.

xmlns:plone="http://namespaces.plone.org/plone"
xmlns:browser="http://namespaces.zope.org/browser">

<include package=".publish" />
Copy link
Member

Choose a reason for hiding this comment

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

ValidationError: ImportError: Module senaite.impress.browser has no global publish

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be part of the next PR, sorry for that. Because I had the package already, it did not fail on my side.

@xispa xispa merged commit 8428525 into 2.x Feb 8, 2023
@xispa xispa deleted the refactor-to-viewlets branch February 8, 2023 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants