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

adds simple logic that keeps authored captions in galleries #15004

Merged
merged 7 commits into from
Jul 31, 2019

Conversation

draganescu
Copy link
Contributor

Description

Attempted fix for #8310

How has this been tested?

  • add a gallery to a post
  • edit captions of images
  • edit the gallery
  • the captions should remain
  • delete a caption or more
  • edit the gallery and add captions to the files which dont have captions
  • the images with empty captions should get the caption from the gallery

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@draganescu
Copy link
Contributor Author

This is not the best fix yet, but it is a quick fix. @noisysocks you had been bitten by the disappearing captions, do you think this approach could work, and later move to calling the REST media api and update the caption for each attachment based on the GTB gallery 's rich text value?

Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

This makes sense to me. I don't think we need to bother with adding a new REST request. Are there any fields other fields other than caption that we should source from the existing selected images?

packages/block-library/src/gallery/edit.js Outdated Show resolved Hide resolved
@kjellr
Copy link
Contributor

kjellr commented Apr 18, 2019

This definitely works better, in that there are no lost captions. That's a massive win on its own. 🙌

There's still a couple weird experiences that occur under the following conditions:

  1. Insert a gallery, add a caption to (at least) one image.
  2. Hit the "Edit" icon to open the media modal.
  3. Add a new, different caption for that image. Hit "Update Gallery"
  4. Notice that the new caption you added is not present, since it's overwritten by the one entered in Gutenberg.

It'd be great if we could offer logic to see if the caption has been changed since the Media modal was opened, and then update the caption in Gutenberg to match. Not sure if that's possible though.

And also:

  1. Insert a gallery, include an image with an existing caption.
  2. In Gutenberg, remove the existing caption.
  3. Hit the "Edit" icon to open the media modal.
  4. Do not make changes, but hit "Update Gallery" to return the Gallery block.
  5. Notice that the original caption from step 1 is now present again.

If you modify (not delete) the caption in Gutenberg, the caption will not be overwritten by the one in the media modal. When comparing to that, the behavior in step 5 is inconsistent. I'd expect the editor to know that if I've manually deleted a caption, it's intentionally gone and should not be replaced so easily.

In general though, I'm super-glad that this means no more lost captions, so it's a definite improvement. 👍

@draganescu
Copy link
Contributor Author

I've updated the code to @noisysocks 's review and also @kjellr I have added the following behaviour:

  • if you edit the caption in the gallery modal it will update the one in GTB

Feels a lot better now, let me know if you can take it for another spin :)

@draganescu draganescu force-pushed the try/save-caption-for-real branch from 16efe35 to c2e280f Compare April 22, 2019 15:07
@draganescu draganescu force-pushed the try/save-caption-for-real branch 2 times, most recently from 55f67f1 to 66c5bb3 Compare May 3, 2019 09:08
@draganescu
Copy link
Contributor Author

draganescu commented May 6, 2019

@gziolo @talldan can you please shine some light here as I can't seem to be able to figure out what is wrong with this failing test:

npm run test-unit -- --testPathPattern test/integration/full-content/full-content.spec.js

The only thing that causes it to fail appears to be the new item in block.js (attachmentCaption). All the other code passes all tests (I tested by applying each change incrementally).

As soon as I add

	"attachmentCaption": {
		"type": "string"
	}

to the block.json the test fails. The error is:

● full post content fixture › core__gallery

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Unknown source type \"undefined\""]

      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:34:4)

  ● full post content fixture › core__gallery__columns

    expect(jest.fn()).not.toHaveErrored(expected)

    Expected mock function not to be called but it was called with:
    ["Unknown source type \"undefined\""]

      32 | 	function assertExpectedCalls() {
      33 | 		if ( spy.assertionsNumber === 0 && spy.mock.calls.length > 0 ) {
    > 34 | 			expect( console ).not[ matcherName ]();
         | 			^
      35 | 		}
      36 | 	}
      37 |

      at Object.assertExpectedCalls (packages/jest-console/src/index.js:34:4)

If you test be sure to run npm run test-unit -- --testPathPattern --clearCache test/integration/full-content/full-content.spec.js to make sure you don't get false positives.

@gziolo
Copy link
Member

gziolo commented May 7, 2019

As soon as I add

	"attachmentCaption": {
		"type": "string"
	}

Isn't it missing source and attribute properties? See how the parent source with value query should be structured: https://github.com/WordPress/gutenberg/blob/master/docs/designers-developers/developers/block-api/block-attributes.md#query

@draganescu draganescu requested review from aduth, nerrad and ntwb as code owners May 7, 2019 09:54
@draganescu
Copy link
Contributor Author

draganescu commented May 7, 2019

Indeed @gziolo that was the issue!

I have refactored the code and now the failing test works.

Instead of adding a caption attribute to each image in the gallery I added an attachmentCaptions array attribute of the gallery ( props @talldan ) and that avoided the need to specify a selector and source.

@oandregal oandregal added the [Block] Gallery Affects the Gallery Block - used to display groups of images label May 8, 2019
@draganescu draganescu force-pushed the try/save-caption-for-real branch 2 times, most recently from 7effb39 to c164a28 Compare May 24, 2019 12:56
@draganescu draganescu force-pushed the try/save-caption-for-real branch from 23cd0a9 to dc5c599 Compare May 29, 2019 10:03
@draganescu draganescu force-pushed the try/save-caption-for-real branch from dc5c599 to c821ca5 Compare June 4, 2019 21:42
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Jun 5, 2019
@gziolo gziolo added the [Feature] Media Anything that impacts the experience of managing media label Jun 5, 2019
@gziolo
Copy link
Member

gziolo commented Jun 5, 2019

I discovered one more issue when testing it. Steps to reproduce:

  • add captions in Media library
  • update captions in Gallery
  • save and reload
  • open Media library for the gallery and click Update Gallery
  • captions get replaced even don't modified

gallery-media

@draganescu draganescu force-pushed the try/save-caption-for-real branch from c821ca5 to 3179f4b Compare July 15, 2019 12:36
@draganescu
Copy link
Contributor Author

@gziolo the bug you found here is not really solvable without tying up the gallery component to the media gallery in WP, as for fixing that we need to monitor the state of the attachments in the modal and only update the component if that state is "dirty".

I would push forward with things as they are, what do you think?

@draganescu draganescu requested review from gziolo and talldan July 18, 2019 10:15
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

It's better than it used to be. Let's move forward 👍

@draganescu draganescu merged commit becdb52 into master Jul 31, 2019
@github-actions github-actions bot added this to the Gutenberg 6.3 milestone Jul 31, 2019
@youknowriad youknowriad deleted the try/save-caption-for-real branch July 31, 2019 12:42
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* adds simple logic that keeps authored captions in galleries

* adds caption updating if changed in gallery, refactores some code

* refactor code so as to avoig adding data to the DOM and updated the fixtures

* updated new deprecated fixture

* removes manual fixture edits

* moves attachment captions to state

* fixed a problem caused by rebasing
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* adds simple logic that keeps authored captions in galleries

* adds caption updating if changed in gallery, refactores some code

* refactor code so as to avoig adding data to the DOM and updated the fixtures

* updated new deprecated fixture

* removes manual fixture edits

* moves attachment captions to state

* fixed a problem caused by rebasing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Gallery Affects the Gallery Block - used to display groups of images [Feature] Media Anything that impacts the experience of managing media [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants