plone
Add ImageWidget with upload/drop/external and inline/widget capabilities
Deploy Preview for plone-components canceled.
| Name | Link |
|---|---|
| Latest commit | dfdabf2618a5bb7660e823ca4f12fa8ead2e9cd4 |
| Latest deploy log | https://app.netlify.com/sites/plone-components/deploys/667670c8e9b3650008f3d25f |
![netlify[bot] avatar](https://avatars.githubusercontent.com/in/13473?v=4 "Published by a pub.dev verified publisher"){kind=small}
Deploy Preview for volto failed.
| Name | Link |
|---|---|
| Latest commit | 3c44490a731b4e64963b331cd8794babefcd13bd |
| Latest deploy log | https://app.netlify.com/sites/volto/deploys/6628a245b1491900090e2c6c |
I have moved the https://github.com/plone/volto/pull/4142 here, because of the conflicts and the big changes since that pr.
I have considered that is better to return always the URL of the image because it will be easier for developers to use if it is consistent. What do you think @sneridagh?
I compared against production docs.
These look the same. Are they supposed to be the same? I'm not sure what I should expect or look for.
- https://65c09ac45778640008535e9c--volto.netlify.app/storybook/?path=/story/view-widgets-image--image
- https://6.docs.plone.org/storybook/?path=/story/view-widgets-image--image
The controls appear broken in the Netlify build:
- https://65c09ac45778640008535e9c--volto.netlify.app/storybook/?path=/story/edit-widgets-object-browser--image
against production:
- https://6.docs.plone.org/storybook/?path=/story/edit-widgets-object-browser--image
Is the difference expected?
Both have a broken image. Should that be fixed?
Please let me know. Thank you!
Hi @stevepiercy, @sneridagh added this for the storybook: https://65c09ac45778640008535e9c--volto.netlify.app/storybook/?path=/story/edit-widgets-image--default, in the old pr, that I have moved here. You can see that shows the Image Widget.
@dobri1408 let's try to push this forward. I just merged, we need to fix the tests. I would also be great that we could address what @tiberiuichim was proposing here as well https://github.com/plone/volto/issues/4286 (the link widget is too much, should happen in another phase, addressed by the other PLIP)
Hello @sneridagh and @davisagli, I've been working on the Image Widget and think it's a good time for a review to ensure it meets the desired requirements and is ready for merging. I've used the same widget for the image block as well.
@dobri1408 I tested it locally and the test failed in a legit way. If you are going to select an image (or any other object) the UX expects that you start browsing whereever you are now, in this case, inside the document (because it's already created, we are editing it). So this is a legit bug. Could you please take a look?
I have another comment, see below
Hi @sneridagh, I've made the changes recommended by @ichim-david and am currently waiting for his review.
@dobri1408 run i18n and commit the change and if necessary update test snapshots. I had to add some aria labels to the AddLinkForm https://github.com/plone/volto/pull/5607/commits/e617d73d5bbe7c3fad9ddd28f7ee55dfd34e34c6 due to them reading "Image button" when using a screen reader
I have added back placeholderLinkInput = '', because there are tests that change the placeholder Link Input
@sneridagh I've removed the brown important color https://github.com/plone/volto/pull/5607/commits/ec7c2eae37970c6ca7f0cb3fc2707aef2636a670#diff-a584ff5e44089465e761b5e7077bbd875f0b92ed9307db7fea91df1240e76e78L404 from .toolbar-inner .ui.icon.button In doing so the icons from the image widget have opacity changes when focused just like it happens when using other object browser widget blocks such as map or video. Have a look at this video and you will see what I mean
https://github.com/plone/volto/assets/152852/482d0659-84ee-4f16-a7b4-9c61f95517e3
I also had to add this rule https://github.com/plone/volto/pull/5607/commits/ec7c2eae37970c6ca7f0cb3fc2707aef2636a670#diff-cf128453bb126c12a2d763cd1afdbaf4d32ba380bd02702d0393f5ea4c666bb1R30
to avoid having a border looking box shadow when focusing on the primary submit button.
Without it since slate-inline-toolbar isn't part of main like our override there it looked like this
@plone/volto-team this is ready for final review
@sneridagh have another look now regarding the placeholder size when added within a grid and the link popup.
I have one thing that bothers me which is the difference in placeholder svg when we have a grid and we mix teasers and image blocks. You can see this issue in this video:
https://github.com/plone/volto/assets/152852/780480c6-fb1c-4a03-b548-fa04a3964f41
If we remove center then things look better but if we only have teasers then they won't look as balanced on the grid area as when the justify-content was set to center. Given the fact that the grid was made primarily for teasers I leave it as it is however I had to add at least a comment that it bothered me the non-alignment of teaser and image placeholder as changing the current behavior would probably annoy other people thinking why the teasers are no longer centered :)
@ichim-david I see... difficult question :/ indeed removing it will look better overall. It's been long time that I wanted to change that (and remove the semanticUI class from there).
I like https://react-spectrum.adobe.com/react-spectrum/IllustratedMessage.html but I don't see that we are doing anything extraordinary like in there.
@ichim-david I can't agree more. Let's merge this, since it was the original purpose of this PR. @dobri1408 @ichim-david Thanks a lot for the contribution, this is awesome, and very much needed. Merging now.