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

Gallery: Duplicate image IDs into easy-to-parse attribute #11540

Merged
merged 11 commits into from Nov 19, 2018

Conversation

@mcsf
Copy link
Contributor

commented Nov 6, 2018

Description

Fixes #8193.
Would supersede #11212.
Prototyped during a call with @mtias.

Warning: yes, this is quite awful. I mainly want to close this unless I can be convinced this is a trade-off worth making.

This is yet another trade-off to allow easy parsing of galleries on the server. This functionality is expected by many, especially given the possibilities offered by the [gallery] shortcode, etc. A comprehensive set of server-side tools for deep block inspection isn't expected until WordPress 5.1, thus in the meantime measures such as this can make third-party developers' lives easier. See parent issue for more.

Conceptually, I don't like what this PR does, as it duplicates state. Indeed, Gallery has an attribute images, an array of image objects whose properties include id. This attribute is sourced in the block's HTML, however, based on elements' data attributes. This PR introduces an attribute ids which is not sourced and contains just the IDs extracted from images.

It highlights my concerns about state duplication:

  • that state needs to be kept in sync with arguably fragile techniques, such as overriding setAttributes to enforce the unidirectional images -> ids synchronisation;
  • that any piece that initialises a Gallery block needs to be aware of this duplication, as highlighted in the changes in transforms — but this would have to be replicated in any other block that offers a transform to a Gallery;
  • this last point would not be properly solved by letting the Gallery constructor set its duplicate attribute automatically, as this would be yet another case of self-induced block changes that put the user in a state of "stuck undo"; it would also break synchronisation if a user somehow isn't in Visual/Block mode upon transforming blocks.
Copy link
Member

left a comment

I'm reasonably okay with this direction. If we have the IDs, I'd like to see images contain as little as possible which is not redundant with the id. I think that would mainly be captions? May even be possible to change attributes shape to ids and captions at that point. Though I suppose we'd probably want to limit such changes at this point in the development cycle.

The mention of a block "constructor" seems very similar to what we'd had early in Gutenberg with the function form of attributes, eventually removed due to its inability to interoperate with the backend (#1905).

ids: {
type: 'array',
shortcode: ( { named: { ids } } ) => {
return ids.split( ',' );

This comment has been minimized.

Copy link
@aduth

aduth Nov 12, 2018

Member

ids is not guaranteed to be assigned, at which point this would throw.

@aduth

This comment has been minimized.

Copy link
Member

commented Nov 12, 2018

eventually removed due to its inability to interoperate with the backend (#1905).

Funny enough we still had at least one lingering reference in our documentation 😄 See #11765

@mtias mtias added this to the 4.4 milestone Nov 12, 2018
@mcsf mcsf referenced this pull request Nov 13, 2018
0 of 4 tasks complete
@youknowriad youknowriad modified the milestones: 4.4, 4.5 Nov 15, 2018

this.state = {
selectedImage: null,
};
}

setAttributes( attributes ) {
if ( attributes.ids ) {
throw new Error( "Don't use this attribute, dangit." );

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 15, 2018

Author Contributor

Note to self: even though this error should never be thrown, let's change the copy to something neutral and descriptive.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-server-readable-ids branch 2 times, most recently from 542187a to 3108056 Nov 15, 2018
Copy link
Member

left a comment

I did some polishing on this PR; it should be ready. I think this approach is a good tradeoff between providing server access to the ids and using our existing mechanisms.
I agree with @aduth, that removing the id data attribute from the image may even be a better path. But I'm not sure if it is something we can do now, and this path seems "safer."

@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-server-readable-ids branch from 3108056 to 7efb944 Nov 15, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-server-readable-ids branch from 7efb944 to 063f643 Nov 15, 2018
@antpb

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

@jorgefilipecosta seems it is failing tests. Can't see why when comparing your changes to the last success...

FAIL test/e2e/specs/links.test.js (54.639s)
  ● Links › allows autocomplete suggestions to be selected with the mouse
    expect(received).not.toBeNull()
    Received: null
      302 | 		// Expect that clicking on the autocomplete suggestion doesn't dismiss the link popover.
      303 | 		await firstSuggestion.click();
    > 304 | 		expect( await page.$( '.editor-url-popover' ) ).not.toBeNull();
          | 		                                                    ^
      305 | 
      306 | 		// Expect the url input value to have been updated with the post url.
      307 | 		const inputValue = await page.evaluate( () => document.querySelector( '.editor-url-input input[aria-label="URL"]' ).value );
      at Object._callee17$ (test/e2e/specs/links.test.js:304:55)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:62:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:288:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:114:21)
      at asyncGeneratorStep (test/e2e/specs/links.test.js:15:103)
      at _next (test/e2e/specs/links.test.js:17:194)
@aduth

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

@antpb This test has been very flakey lately. Trying rebuild..

@antpb

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

that did it @aduth ! Thanks! Worth noting here, this PR will close #1451 as well. The main thing needed there is a friendly way to consume ids

Copy link
Contributor Author

left a comment

I suggested a couple of nit-picks, but LGTM :shipit:

packages/block-library/src/gallery/index.js Outdated Show resolved Hide resolved
packages/block-library/src/gallery/index.js Outdated Show resolved Hide resolved
packages/block-library/src/gallery/index.js Outdated Show resolved Hide resolved
packages/block-library/src/gallery/index.js Outdated Show resolved Hide resolved
mcsf and others added 4 commits Nov 19, 2018
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>
@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-server-readable-ids branch 2 times, most recently from f30b663 to 1815336 Nov 19, 2018
@jorgefilipecosta jorgefilipecosta force-pushed the fix/gallery-server-readable-ids branch from 1815336 to 526b956 Nov 19, 2018
@jorgefilipecosta

This comment has been minimized.

Copy link
Member

commented Nov 19, 2018

🚫 Actually, this still needs a deprecation for Gallery.

Hi @mcsf, in my tests I did not find any valid gallery created on the previous version that becomes invalid after this changes.
What I found was that we did not automatically add ids attribute to previous existing galleries, in order to improve that I used isEligible and added a migration.
This improvement also adds some nice features e.g. if we remove an image in the code (delete its li element) this mechanism also deletes the id attribute from the array, basically the mechanism makes sure images.id and ids are always in sync even if changed using the code editor.
I'm not sure if the reason you referred the need for deprecation was this one, if that is not the case feel free to let me know and I will update the logic.

Copy link
Contributor

left a comment

The code looks good to me, and I wasn't able to trigger the deprecation error as well.

@antpb

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2018

I see a lot of green statuses here. 🙂 With @youknowriad last comment I feel good with us moving forward on merging this one. Thank you @mcsf ! Fixes: #1451 and #8193

@antpb antpb merged commit e601299 into master Nov 19, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Extensibility automation moved this from In Progress to Done Nov 19, 2018
@mcsf mcsf deleted the fix/gallery-server-readable-ids branch Nov 19, 2018
@mcsf

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2018

I'm not sure if the reason you referred the need for deprecation was this one, if that is not the case feel free to let me know and I will update the logic.

Actually, it was a quick phone-side comment and I didn't think that deeply, but this is great! Thanks for the work, @jorgefilipecosta!

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018
…11540)

* [TRY] Gallery: Duplicate images' IDs into easy-to-parse attribute

* typo fix

* Fixed a check in shortcode transform; Handled the file transform.

* Corrected fixtures and snapshots.

* Improved error message.

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Add deprecation logic.

* update fixtures
grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Jan 4, 2019
…11540)

* [TRY] Gallery: Duplicate images' IDs into easy-to-parse attribute

* typo fix

* Fixed a check in shortcode transform; Handled the file transform.

* Corrected fixtures and snapshots.

* Improved error message.

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Update packages/block-library/src/gallery/index.js

Co-Authored-By: jorgefilipecosta <jorge.costa@developer.pt>

* Add deprecation logic.

* update fixtures
@selftripsdotru

This comment has been minimized.

Copy link

commented Feb 10, 2019

Sorry, but problem is solved or not?

@noisysocks

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

@selftripsdotru: Yes, this PR was merged and shipped as part of WordPress 5.0.

@selftripsdotru

This comment has been minimized.

Copy link

commented Feb 11, 2019

What is PR?
In the WordPress 5.0 or more, the problem is!!

@selftripsdotru

This comment has been minimized.

Copy link

commented Feb 11, 2019

How to get new IDs for galleries after export/import?

@mcsf

This comment has been minimized.

Copy link
Contributor Author

commented Feb 11, 2019

What is PR?

A PR is a pull request, such as the one on this page. It means that this change has been incorporated in WordPress 5.0.

How to get new IDs for galleries after export/import?

For general help requests, please post in the support forum at https://wordpress.org/support/forum/how-to-and-troubleshooting/.

Technical help requests have their own section of the support forum at https://wordpress.org/support/forum/wp-advanced/.

You may also ask for technical support at https://wordpress.stackexchange.com/.

Please make sure you have checked the Handbook at https://wordpress.org/gutenberg/handbook before asking your question.

@gziolo gziolo referenced this pull request May 7, 2019
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.