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

Display document byline viewlet only to anonymous users #90

Merged
merged 2 commits into from
Jun 2, 2016

Conversation

hvelarde
Copy link
Member

@hvelarde hvelarde commented May 5, 2016

Also, clean up the document byline code to remove unnecessary code.

If permited by the Allow anyone to view 'about' information option in the Security Settings of Site Setup.

Refs: plone/Products.CMFPlone#1556
@hvelarde
Copy link
Member Author

hvelarde commented May 5, 2016

@davilima6 @idgserpro @agnogueira @rodfersou please review and comment.

next step should be work on solving plone/Products.CMFPlone#668

@@ -71,7 +71,7 @@ def show(self):
ISecuritySchema,
prefix='plone',
)
return not self.anonymous or settings.allow_anon_views_about
return self.anonymous and settings.allow_anon_views_about
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to add a test so we don't get a regression in this important line again. Where do you think it would be the best place?

@davilima6
Copy link
Member

davilima6 commented May 5, 2016

Since we're assuming "byline is metadata, therefore content", won't it get inconsistent UX-wise to have it displayed differently between anon and logged-in states? Besides it may also break themes because an entire block (and its styles) will be missing, so one more thing to beware of...

My question being: isn't it less riskier to live with the toolbar redundancy (+1 here) or should we just wait and see if these problems really happen and actually bother users? @agnogueira, @idgserpro: opinions?

@@ -71,43 +71,7 @@ def show(self):
ISecuritySchema,
prefix='plone',
)
return not self.anonymous or settings.allow_anon_views_about
Copy link
Member

Choose a reason for hiding this comment

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

Why not showing for non-anonymous too?
Only reason I can think of is, that the some of the information is available in the toolbar too. But is the toolbar also visible for simple members without editing rights? Then we need this viewlet also for them.

Copy link
Member

@davilima6 davilima6 May 5, 2016

Choose a reason for hiding this comment

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

Yes, the clock icon is available to Members.

Copy link
Member Author

@hvelarde hvelarde May 5, 2016

Choose a reason for hiding this comment

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

@thet well, another proof that the toolbar design was half baked and the removal of the viewlet was a bad idea in the first place; I would prefer to leave the document byline visible for all, but in the past we had some heated discussions as some people simply wanted to get rid of it without thinking on all the unintended consequences. I was trying to move on but, yes, you are right.

IMO the document byline helps to remove the cluttering of the toolbar and there are many regressions caused by its removal.

@davilima6 the clock icon is not solving all cases; see plone/Products.CMFPlone#1575

…ment byline, as this information was not available to anonymous users anyway

We need to review if this is available in the current implementation of the toolbar.
@jensens jensens merged commit 5e486e8 into master Jun 2, 2016
@jensens jensens deleted the issue_1556 branch June 2, 2016 23:06
talarias pushed a commit that referenced this pull request Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants