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

Cleanup and rework: contenttype-icons and showing thumbnails for images/leadimages in listings ... #1226

Closed
fgrcon opened this issue Nov 5, 2015 · 17 comments

Comments

@fgrcon
Copy link
Member

fgrcon commented Nov 5, 2015

Replaces #1151, #1074 and adresses #1185 and may be some more.
**Detailed documentation: https://github.com/fgrcon/Products.CMFPlone/blob/documented/docs/icons_in_plone5.rst

I will reopen pull requests when finished (almost done) and tested (as far as I can).
PRs are interdependend

@jensens:
you suggested:
nterdependent pull request are not directly possible by now, this was discussed here plone/jenkins.plone.org#126 . But you can make a branch and PR (which is only for testing, not for merge) in buildout.coredev with adjusted sources.cfg - this can then be tested with the PR>mechanism. Its a workaround , but well...

I created a branch in coredev (see above) but how to start Jenkins with that? I created plone/buildout.coredev#140 -> http://jenkins.plone.org/job/pull-request-5.0/529/ which obviously did not fetch sources from fgrcon....

@gforcada
Copy link
Sponsor Member

gforcada commented Nov 5, 2015

@fgrcon interdependent pull requests can be tested now: just put all of them (one per line) on the jenkins job for either 5.0 or 4.3.

Have fun!

@fgrcon
Copy link
Member Author

fgrcon commented Nov 5, 2015

@hvelarde
Copy link
Member

hvelarde commented Nov 5, 2015

@fgrcon IMO, is a bad idea to include different issues on the scope of one PR; this could lead easily to errors and could delay the merging of it.

@fgrcon fgrcon closed this as completed Nov 5, 2015
@fgrcon
Copy link
Member Author

fgrcon commented Nov 5, 2015

@hvelarde you are absolutely right on that, but this really was not my own decision. I just wanted to contribute a small feature, but i found a lot of dependend issues .... and if you think you can handle this better: i will suport you as much as i can, No need to merge my proposals - but just replace it with other ones ...

@fgrcon fgrcon reopened this Nov 5, 2015
@fgrcon
Copy link
Member Author

fgrcon commented Nov 5, 2015

sorry i dn't close this intentionally,(one click to much ?)

@fgrcon fgrcon closed this as completed Nov 5, 2015
@fgrcon fgrcon reopened this Nov 5, 2015
@hvelarde
Copy link
Member

hvelarde commented Nov 5, 2015

sorry, I have no time, neither knowledge to do what you're doing; I'm just giving my opinion.

something is rotten in the state of Plone if the contribution of a small feature turned into a nightmare of dependent issues; take some fresh air and think about it again...

@jensens
Copy link
Sponsor Member

jensens commented Nov 6, 2015

@hvelarde yes, this is indeed one little thing distributed all over the place. I'am not sure if this states that Plone is rotten. It's in the nature of things that icons are used at several places. The only thing to criticize is that the original change from v4 to v5 on how icons are used was not implemented completely. Here our quality assurance failed massively.

And I am really happy that @fgrcon took the time and energy to fix this. Thank you @fgrcon!

I'll try to look at the PR today and merge if possible.

@fgrcon
Copy link
Member Author

fgrcon commented Nov 6, 2015

http://jenkins.plone.org/job/pull-request-5.0/532/ (with all prs)
@jensens @hvelarde @vangheem ....
I dont think that plone is such a mess (a long history leeds to some traces, compared to what i have seen since plone 3 the whole process and plone itself have improved dramatically. But of course there always is room for further improvement.)

yes this is quite a time consuming and enervating job and still not finished yet.:

  • passing all the tests
  • still a fix needed in mockup ( replace portal_type.toLowerCase() with a js implementation of normalizeString ..
  • an upgradestep ( reindexing of get_icon) ?? (i am not familiar with upgrade steps yet)
  • maybe some fixes in barceloneta as well (contenttype-icons dont show up in related items browser dropdowns, to many ugly bullets in some portlet views ..)

But i think it would be great to get the major things into core with your help asap. (I am getting a little bit tired of rebasing and have a lot of other things in mind with plone)
see https://github.com/fgrcon/Products.CMFPlone/blob/master/docs/icons_in_plone5.rst . I am just adding some more overview info there.

@hvelarde
Copy link
Member

hvelarde commented Nov 6, 2015

don't take my words too seriously, guys; it's obvious that I don't have all the details but... yes, we have a problem with features distributed among dependencies and that's something we have to fix.

@fgrcon
Copy link
Member Author

fgrcon commented Nov 9, 2015

http://jenkins.plone.org/job/pull-request-5.0/536 (with all 8 prs)
needed to comment out some tests in mockup/tests/pattern-relateditems-test.js in order to get the build done. These testcases seem to need rework and as far as I can see by a number of manual tests they show false results or i dont understand them.
2 small fixes ..: http://jenkins.plone.org/job/pull-request-5.0/537/

@gforcada
Copy link
Sponsor Member

gforcada commented Nov 9, 2015

@fgrcon down to only one error in p.a.event! You are almost there!

@fgrcon
Copy link
Member Author

fgrcon commented Nov 10, 2015

@gforcada yes live can be hard and cruel ;-)
http://jenkins.plone.org/job/pull-request-5.0/539

@vangheem
Copy link
Member

test passed, everything merged. @fgrcon can you make sure I didn't miss anything?

Thanks for your work on this.

@fgrcon
Copy link
Member Author

fgrcon commented Nov 11, 2015

@vangheem only two more left: #1227
and plone/plone.app.event#205

btw: the metadatafield get_icon requires an index rebuild. How can this be done (ipgrade-step?)

@hvelarde
Copy link
Member

great job, indeed!

@jensens
Copy link
Sponsor Member

jensens commented Nov 11, 2015

🎆 well done!

@jensens
Copy link
Sponsor Member

jensens commented Nov 11, 2015

The upgrade step should go into plone.app.upgrade. I think there are some examples in there, i.e. this one https://github.com/plone/plone.app.upgrade/blob/master/plone/app/upgrade/v43/alphas.py#L14
It goes a bit deep into catalog internals, but does it in an efficient way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants