plone / plone.app.widgets

Integrating plone.widgets into plone
Other
7 stars 38 forks source link

Numerous Mockup 2 compatibility fixes for 1.x branch, plus semver and TinyMCE fixes #144

Closed seanupton closed 7 years ago

seanupton commented 7 years ago
seanupton commented 7 years ago

@thet care to take a look when you have some bit of time?

seanupton commented 7 years ago

Quick comment: I am expecting to add a commit to this PR's fork/branch today fixing the upload pattern progress bar missing LESS import (this would get fixed in mockup master, then I would copy static assets into a commit here).

I am waiting on feedback on UX question of whether we really want to show or hide the progress (percentage) label. Template currently attempts to hide, but CSS does not hide the boilerplate message ("40% complete...").

Edit: punting on this question for now, just making sure that Bootstrap defaults are used (by careful import). See: https://github.com/plone/mockup/pull/698

frisi commented 7 years ago

hi @seanupton great to see some efforts in making p.a.widgets more interesting for plone4.3 i added it to a project recently and now i'm considering to get rid of it again because of the poor tiny-integration. ie: image scales are not read from the site settings but hard-coded, image-caption option is gone.

do you plan to change/fix this too? do you think this can be done within a few hours? if yes and you can point me to the right directions we'd spend a few hours on this.

otherwhise i'd either get rid of p.a.widgets completely again (mainly used for select2 autocomplete) or i'd try to make a fork that does not replace the tiny widgets in the at_bbb and dx_bbb modules, does not include pat-tiny and does not deprecate the tiny java-script files shipped with plone to be able to use good old tinymce with a working image-dialog

seanupton commented 7 years ago

Added newer mockup widgets bundle in https://github.com/plone/plone.app.widgets/pull/144/commits/7c5ea34b07d10acba53c1ce53f4797214b6a2a62 to fix broken icons on related items for Plone 4 integration.

seanupton commented 7 years ago

@frisi -- on the image scales, I have this working AFAICT. I chose an image, let it use the default scale. Then I edited image in TinyMCE, chose thumb scale, and it directed to appopriate scale URL on save.

Try temporarily using https://github.com/upiq/plone.app.widgets.git in your sources and see if this works for you?

seanupton commented 7 years ago

@frisi another TinyMCE win from just using newer TinyMCE and newer mockup (2) is better table editing. IMHO, the TinyMCE integration is now working well enough for me to deploy to my users with expectation that they will appreciate the improvement over TinyMCE 3, very much.

seanupton commented 7 years ago

To get static text portlet working as we would want in Plone 4 with plone.app.widgets 1.x, we also need to get https://github.com/plone/plone.portlet.static/pull/12 merged.

coveralls commented 7 years ago

Coverage Status

Changes Unknown when pulling 1b8fbfb021439dcc2795b7e737a0904309d93892 on upiq:1.x into \ on plone:1.x**.

jensens commented 7 years ago

@seanupton LGTM - is there more work needed or is it ready for merge?

seanupton commented 7 years ago

UPDATE: before any merge, should I try to integrate @thet's latest related items updates to mockup? Could be beneficial, and I'm willing to test in Plone 4 context.

@thet @jensens This should be good to merge. I've had these changes in production on 4.3.x sites for 2 weeks with no complaints.

Working, uniformly configured TinyMCE everywhere (when paired with plone.portlet.static 3.1) is nice to have, among the other improvements.

seanupton commented 7 years ago

And on that note, the only thing I have not tested that I suspect is not working is embedding video in TinyMCE in Plone 4; that should not be a blocker on a merge.

Maybe I can try integrating latest mockup (including related items changes by @thet), testing in Plone 4. If this works and gets merged, I think that's a good time for a 1.9.1 release.

jochumdev commented 7 years ago

@seanupton at first thanks for you'r hard work on this.

@fristi told me to try your TinyMCE changes here, sadly i can't see our scales for images in it.

Do i understand you right, this branch should bring that? I don't need to install any upgrades?

Can you help me with what i'm doing wrong?

seanupton commented 7 years ago

@pcdummy @frisi try removing and re-adding (internal) image.

Does that change whether scales work for you?

There could be an issue of backward-content-compatibility with images inserted using TinyMCE 3 (Products.TinyMCE). I have no trouble with scales on newly added images; I have no trouble adjusting scales in an internal image placed in TinyMCE in a new document.

Images added by Products.TinyMCE on existing Plone 4.3 site previously now upgraded to plone.app.widgets 1.x: all images are shown as "external" with relative path. plone.app.widgets and mockup TinyMCE use resolveuid/{UID} style src attributes, not relative path to content.

If this is your issue, I don't see this as a blocker to merging or releasing, unless something else is going on.

This may be something I have to consider in my own site migrations, though (rewriting content).

frisi commented 7 years ago

@seanupton thanks for your clarification on the behaviour with existing text containing images.

although what bothered us is something different: i asked @pcdummy to try out whether one can select the scales defined in the plone.app.imaging settings in the image dialog (so also scales added by integrators). or is the tiny image dialog limited to the scales defined in js/python code - instead of site configuration (getAvailableSizes)?

seanupton commented 7 years ago

@frisi @pcdummy The scales (for Plone 4 and plone.app.widgets 1.x) come from the control panel: https://snag.gy/yxk7nZ.jpg

jochumdev commented 7 years ago

@seanupton for me they don't.

aida-scales-tinymce aida-scales

jochumdev commented 7 years ago

While greping through p/a/widgets/static/widget.js i found scales staticaly defined (line 105076).

We have been talking howto solve TinyMCE integration solutions:

jochumdev commented 7 years ago

This is how an image line looks in Tiny2:

<img class="image-inline captioned" src="resolveuid/f5ec83e6905247b99b258eaff3d140ee/@@images/image/article-full" />

This is how it should look like in Tiny3:

<img class="image-inline captioned" src="resolveuid/f5ec83e6905247b99b258eaff3d140ee/@@images/image/preview" data-linktype="image" data-scale="preview" data-val="f5ec83e6905247b99b258eaff3d140ee" />```
seanupton commented 7 years ago

@pcdummy what you are seeing in mockup source are default values, and options can be passed; however, it appears that you are correct, and I was mistaken. In the 1.x branch, plone.app.widgets.utils.get_tinymce_options() has never yet used plone.app.imaging.utils.getAllowedSizes(), like Products.TinyMCE does.

While this is not a regression re: where plone.app.widgets was before, I think we can look into adding scales defined with very little effort.

My other item of work for today is integrating @thet's related items widget improvements, but I may need some minor compatibility fixes on that front (getting some benign errors in the console).

frisi commented 7 years ago

it would be great if you can make the scales defined in plone.app.imaging available in tiny @seanupton. are you working on that already?

i asked on https://community.plone.org/t/images-inserted-with-plone4-broken-on-plone5/2722 if there is existing code that fixes the tiny image editor for images added with Products.TinyMCE (i think you meant something similar in your comment https://github.com/plone/plone.app.widgets/pull/144#issuecomment-245636770) if there is no code yet, we can provide some script that can be used as basis for custom migrations

frisi commented 7 years ago

the image dialog ist working for dx and with #146 also for archetypes (in case tiny has been configured to use uids) see https://community.plone.org/t/images-inserted-with-plone4-broken-on-plone5/2722/5?u=frisi

@seanupton are you finished with the related items fixes? i think it would be the best to get this and #146 merged asap.

the image-scale thing can be done in another pr. if you can find the time for that in the next days that would be awesome.

thet commented 7 years ago

@seanupton - is your integration and backporting work done? than this can be merged, IMO.

not sure about tinymce image scales... i fixed that in mockup and cmfplone. might need the cmfplone fix in plone.app.widgets too. see:

seanupton commented 7 years ago

@thet I'm considering my work done enough for release. The only thing that was holding me up, and I am willing to punt on is wanting to fix the TypeError in the breadcrumbs in related items in mockup master. That, however, is not a blocker for this, because that exception does not prevent use of the widget (in either Plone 5 or Plone 4).

Please merge, if you feel like it. A release might be good soon (hopefully, you are okay with my slight tweak to the semver numbering).

thet commented 7 years ago

Tnx for all the fixes! I merged it. General note: plone.app.widgets 2.x options should also be cleaned up...

janga1997 commented 7 years ago

@seanupton @frapell I would like to discuss with you about the gsoc project you are mentoring related to this repository. Where might be an appropriate place to contact you both?