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

Add option tho show thumbs for images in content listing and rename-p… #592

Closed
wants to merge 5 commits into from
Closed

Conversation

fgrcon
Copy link
Member

@fgrcon fgrcon commented Oct 6, 2015

…opover

plone/Products.CMFPlone#1074

Please review as I am quite an absolut beginner

@vangheem
Copy link
Member

vangheem commented Oct 7, 2015

This looks fine. Only comment I have is that I think the key "Thumb" should be lowercase "thumb".

I'll leave this open until we merge the plone.app.content PR.

@jensens
Copy link
Member

jensens commented Oct 7, 2015

@fgrcon before we can merge your pull request you need to sign the contributor agreement. see https://plone.org/foundation/contributors-agreement
If you already did and accidentally not appear in the list of contributors please send an email to [email protected] in order to clarify.

@fgrcon
Copy link
Member Author

fgrcon commented Oct 7, 2015

agreement submitted

dropdownCssClass: 'pattern-relateditems-dropdown',
maximumSelectionSize: -1,
resultTemplate: '' +
'<div class="pattern-relateditems-result pattern-relateditems-type-<%= portal_type %> <% if (selected) { %>pattern-relateditems-active<% } %>">' +
' <a href="#" class="pattern-relateditems-result-select <% if (selectable) { %>selectable<% } %> contenttype-<%= portal_type.toLowerCase() %>">' +
' <% if (typeof getIcon !== "undefined" && getIcon) { %><span class="pattern-relateditems-result-icon"><img src="<%= getIcon %>" /></span><% } %>' +
Copy link
Member

Choose a reason for hiding this comment

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

Should we use "thumb" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@vangheem
I am afraid, I don't yets see clear through all this code.

What i tried is
...

<span class="pattern-relateditems-result-title contenttype-<%= portal_type.toLowerCase() %>"><%= Title %></span>'

which is a hack: it should work for 'Folder', 'Image' but of course not for 'News Item' or other (custom ...).
(need something like normalizeString here, or add contenttype-class to vocabulary?

but also classes like 'contenttype-folder' are not respected (need to fix in barcelonetta less?)

using thumb here would be a good and easy solution. But:

  • images (64x64) as i implemented it for folder contents might be too big for the dropdown
  • we should prevent rendering class=contenttype-something together with thumb ..
    so something like:

``` `
if not thumb: class=contenttype.something
else

Copy link
Member Author

Choose a reason for hiding this comment

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

@vangheem
update: I think i solved that ... works fine with thumb... :-) - work in progress

@fgrcon
Copy link
Member Author

fgrcon commented Oct 20, 2015

@vangheem : images and files (and probably other custom types as well) do not have workflows. They return null. Thats why i used typeof != undefined ....

as far as I understand the code now:
folderish did not come from catalog:

defaults:{
...
folderTypes: ['Folder']
...

I replaced that with is_folderish from catalog in order to support all folderish types.
needed to add missing atributes in tinymce patterns as well.

This raises the question wether we could forget the "contains objects" option in tinymce-controlpanel generally. I think this would make things much more simple.

still work in progress ...

@fgrcon
Copy link
Member Author

fgrcon commented Oct 20, 2015

works fine now (provided all related prs see plone/Products.CMFPlone#1151) : tested with relationlist and relationchoice fields etc. custom folderish types,...

Still two minor flaws:

  • new icons dont show up in the relateditems-dropdowns yet
  • no css-classes for new contenttype-iconc and thumbs in tinymce insert image drodowns yet.

@vangheem: ** How to go on ? **
I don't know how to handle the Travis results https://travis-ci.org/plone/mockup/builds/86416152

@@ -99,27 +99,30 @@ define([
homeText: _t('home'),
folderTypes: ['Folder'],
selectableTypes: null, // null means everything is selectable, otherwise a list of strings to match types that are selectable
attributes: ['UID', 'Title', 'portal_type', 'path', 'getIcon'],
attributes: ['UID', 'Title', 'portal_type', 'path','getURL', 'getIcon','contenttype_class','is_folderish','review_state'],
Copy link
Member

Choose a reason for hiding this comment

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

What is "contenttype_class" here?

Copy link
Member Author

Choose a reason for hiding this comment

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

contenttype_class: the css class to be rendered for a contentttype.

the existing code passes over portal_type from vocabulary, then I have seen approaches like portal.type.to:LowerCase() to render the css class. This works sometimes but not for 'News Item' or other (custom dx) types. Of course you could construct some js solution, but the easiest and cleanest way is to use 'contenttype-'+normalizeString(portal_type) which guarentees conformity even when someone alters the implementation of normalizeString. see ...

      if key == 'contenttype_class':
+                        pt = getattr(vocab_item, 'portal_type', None)
+                        val = 'contenttype-%s' % normalizeString(pt)
                     item[key] = val

in plone/plone.app.content#57 : plone/app/content/browser/vocabulary.py

If this is a bad solution, or I am totally wrong, or you think we should just rename contenttype_class ( contenttype_css,
contenttype_css-class, or whatever) pleas let me know.

Copy link
Member

Choose a reason for hiding this comment

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

attributes are meant to be metadata in the catalog. It seems like an abuse to do those custom non-metadata fields in python. It can just as easily be calculated in javascript. We can add a utility method to the utils.js file if we need it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for that, i Just extrapolated from p.a.content/vocabulary.py

                    if key == 'path':
                        attr = 'getPath'
                    val = getattr(vocab_item, attr, None)
                    if callable(val):
                        if attr in _safe_callable_metadata:
                            val = val()
                        else:
                            continue
                    if key == 'path':
                        val = val[len(base_path):]

and i suggested :

                   if key == 'thumb':
                        # geticon redefined in Plone > 5.0
                        # see https://github.com/plone/Products.CMFPlone/issues/1151
                        #     https://github.com/plone/Products.CMFPlone/issues/1074
                        has_image = getattr(vocab_item, 'getIcon', None)
                        if has_image:
                            val = os.path.join(vocab_item.getURL(),'@@images','image','tile')

which you did not object so I thought that was a good way to provide the css class name from there. It is more or less just a derived meta data value and would save us to mimic normalizeString in js (double maintainenance ?).

If you prefer the js variant, I will need your help - my js expertise is not yet sufficient.

BTW: val = os.path.join(vocab_item.getURL(),'@@images','image','tile') could be changed to a boolean value and be expanded in the templates accordingly (getURL+'/@@images/...'

Copy link
Member

Choose a reason for hiding this comment

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

Yah, I was willing to let one go but it seems like a slippery slope :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I dont think this is that slippery (what i can solve with low effort serverside ..), and btw: Iam used to steep slopes in Austria and in Software ;-)
But waiting for your suggestions ...

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest adding a function here: https://github.com/plone/mockup/blob/master/mockup/js/utils.js to format portal_type class and that we try to use that everywhere in the js when we want font-icons to work. Does that help?

@vangheem
Copy link
Member

I don't quite get why getIcon is used in some places and "thumb" is used in others. This needs to be unified. I imagine the plone.app.content PR needs cleaning up in this respect also.

@fgrcon
Copy link
Member Author

fgrcon commented Oct 22, 2015

getIcon vs thumb, ....
(i only wanted to contribute a small feature but obviously I stirred up a hornet's nest with getIcon)

yes i agree, - you mentioned before, that it is not possible to rename meta data field getIcon in minor releases easilyly (getIcon is confusing in the new meaning: icon now means css:before + fontello..., thumb/geticon should mean small scaled image, it would be better to call it thumb (true/false) ).

If you agree, i can change thumb to getIcon everywhere (save titles in contentview) with sufficient commenting until we can change the name to eg. thumb /hasImage/or what else.

I think it also would be a good idea to provide some understandable documentation - any hint where to place that best?

@vangheem
Copy link
Member

Yes, I think that is fine. And thank you for your contributions! I apologize if it's been confusing.

Maybe add docs here? http://docs.plone.org/develop/plone/searching_and_indexing/indexing.html

@fgrcon
Copy link
Member Author

fgrcon commented Oct 22, 2015

thanks I ll try to contribute some documentaion over the next days

no need to apologize, sorry I'm still a plone newbie and plone can be confusing some times - will takesome time :_)

@vangheem
Copy link
Member

Well, you're doing great for a newbie. Digging in and solving problems. It's expected to get redirected some.

Add option tho show thumbs for images in content listing and rename-popover
plone/Products.CMFPlone#1074
fixes to related items

fix...
… be considered browsable. (["Folder"])

does not respect other folderish types: needs to be implemented with meta data field: is_folderish from vocabulary
@fgrcon
Copy link
Member Author

fgrcon commented Nov 5, 2015

temporary closed see: plone/Products.CMFPlone#1226

@fgrcon fgrcon closed this Nov 5, 2015
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.

3 participants