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

@mcsf mcsf 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.

@mcsf mcsf added [Type] Question Questions about the design or development of the editor. [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Feature] Blocks Overall functionality of blocks [Feature] Extensibility The ability to extend blocks or the editing experience [Block] Gallery Affects the Gallery Block - used to display groups of images and removed [Type] Question Questions about the design or development of the editor. labels Nov 6, 2018
@mtias mtias requested a review from a team November 6, 2018 16:04
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

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( ',' );
Copy link
Member

Choose a reason for hiding this comment

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

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

@aduth
Copy link
Member

aduth 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
@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." );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 Compare November 15, 2018 18:55
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

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."

@antpb
Copy link
Contributor

antpb 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
Copy link
Member

aduth commented Nov 15, 2018

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

@antpb
Copy link
Contributor

antpb 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

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

mcsf commented Nov 16, 2018

🚫 Actually, this still needs a deprecation for Gallery.

@gziolo gziolo added [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress labels Nov 19, 2018
mcsf and others added 4 commits November 19, 2018 12:02
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 Compare November 19, 2018 13:03
@jorgefilipecosta
Copy link
Member

🚫 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

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

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

@antpb
Copy link
Contributor

antpb 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
@mcsf mcsf deleted the fix/gallery-server-readable-ids branch November 19, 2018 16:30
@mcsf
Copy link
Contributor Author

mcsf 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!

@selftripsdotru
Copy link

Sorry, but problem is solved or not?

@noisysocks
Copy link
Member

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

@selftripsdotru
Copy link

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

@selftripsdotru
Copy link

selftripsdotru commented Feb 11, 2019

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

@mcsf
Copy link
Contributor Author

mcsf 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.

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] Blocks Overall functionality of blocks [Feature] Extensibility The ability to extend blocks or the editing experience [Priority] High Used to indicate top priority items that need quick attention [Status] In Progress Tracking issues with work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Core blocks Image and Gallery should provide easy access to their data
9 participants