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.

Sign up for GitHub

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

Gallerys ids are saved as numbers #19163

Merged
merged 12 commits into from
Jan 7, 2020
Merged

Gallerys ids are saved as numbers #19163

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
da3f096
Make sure gallery ids are always numbers.
SergioEstevao Dec 16, 2019
1d3582f
Implement deprecation path from post where ids can be strings.
SergioEstevao Dec 16, 2019
0ee6636
Improve is eligible method to apply only when strings are in the ids …
SergioEstevao Dec 16, 2019
514bce7
Remove spaces on empty arrays.
SergioEstevao Dec 16, 2019
ab5f0d2
Remove spaces on empty arrays.
SergioEstevao Dec 16, 2019
41e8c25
Fix indentation.
SergioEstevao Dec 18, 2019
4587f69
Fix indent spaces.
SergioEstevao Dec 18, 2019
5494893
Merge branch 'master' into rnmobile/gallerys_ids_type
SergioEstevao Jan 6, 2020
b3a90fe
Add deprecation test for gallery with string ids.
SergioEstevao Jan 6, 2020
b0211d7
Use some instead of reduce.
SergioEstevao Jan 6, 2020
fc4d8bb
Merge branch 'master' into rnmobile/gallerys_ids_type
SergioEstevao Jan 7, 2020
b40dca1
Simplify the code for parsing id.
SergioEstevao Jan 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions packages/block-library/src/gallery/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
"attributes": {
"images": {
"type": "array",
"default": [ ],
"default": [],
"source": "query",
"selector": ".blocks-gallery-item",
"query": {
Expand Down Expand Up @@ -43,7 +43,10 @@
},
"ids": {
"type": "array",
"default": [ ]
"items": {
"type": "number"
},
"default": []
},
"columns": {
"type": "number"
Expand Down
133 changes: 133 additions & 0 deletions packages/block-library/src/gallery/deprecated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,139 @@ import { RichText } from '@wordpress/block-editor';
import { defaultColumnsNumber } from './shared';

const deprecated = [
{
attributes: {
Copy link
Member

Choose a reason for hiding this comment

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

Is there any opportunity for code reuse in any of these properties or functions? Merging attributes using Object.assign / spread? Reference a common save function?

This is a lot of code to coerce an array to a typed array.

Copy link
Contributor Author

@SergioEstevao SergioEstevao Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@aduth I considerer shared code with the current version of save, but then I saw this comment on the block deprecations documentation:

It’s important to note that attributes, supports, and save are not automatically inherited from the current version, since they can impact parsing and serialization of a block, so they must be defined on the deprecated object in order to be processed during a migration.

Is this still a valid concern? Or I can share code without problems with the current version of save?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is that if you inherit from the current attributes, the deprecations may work now but not later if more attributes are added to the block. If you add an optional thingy attribute to the current block, and your deprecations inherit the entire attributes object from the current implementation, then all the deprecations will stop working. If you change an existing attribute, you have to remember to modify every single deprecation to stop inheriting from the current implementation.

Hardcoding the deprecation definitions, although verbose, is a more robust solution. I don't really see it as much of problem since the deprecations are now usually put in a separate file from the rest of the block data.

Copy link
Contributor

@talldan talldan Dec 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I prefer the longer safer option. Though it's generally ok if there are tests in place.

Which is something I think should be considered for this PR, ideally a core__gallery__deprecated-4.html fixture would be added in this folder:
https://github.com/WordPress/gutenberg/tree/master/packages/e2e-tests/fixtures/blocks

The way I generally do this is:

  1. Check out a branch with the right version of the block for the deprecation (so the gallery block from master)
  2. Insert it into a post
  3. Copy the markup using the Code Editor view
  4. Switch back to this branch
  5. Paste it into a new html file core__gallery__deprecated-4.html in the packages/e2e-tests/fixtures/blocks folder.
  6. Generate the other fixture files by running GENERATE_MISSING_FIXTURES=y npm run test-unit test/integration/full-content/full-content.test.js
  7. Commit and push.

images: {
type: 'array',
default: [],
source: 'query',
selector: '.blocks-gallery-item',
query: {
url: {
source: 'attribute',
selector: 'img',
attribute: 'src',
},
fullUrl: {
source: 'attribute',
selector: 'img',
attribute: 'data-full-url',
},
link: {
source: 'attribute',
selector: 'img',
attribute: 'data-link',
},
alt: {
source: 'attribute',
selector: 'img',
attribute: 'alt',
default: '',
},
id: {
source: 'attribute',
selector: 'img',
attribute: 'data-id',
},
caption: {
type: 'string',
source: 'html',
selector: '.blocks-gallery-item__caption',
},
},
},
ids: {
type: 'array',
default: [],
},
columns: {
type: 'number',
},
caption: {
type: 'string',
source: 'html',
selector: '.blocks-gallery-caption',
},
imageCrop: {
type: 'boolean',
default: true,
},
linkTo: {
type: 'string',
default: 'none',
},
},
isEligible( { ids } ) {
return ids &&
ids.length > 0 &&
ids.reduce( ( state, id ) => {
return state || ( typeof id === 'string' );
}, false );
},
migrate( attributes ) {
return {
...attributes,
ids: map( attributes.ids, ( id ) => {
if ( ! id ) {
return null;
}
if ( typeof id === 'string' ) {
return parseInt( id, 10 );
}
if ( typeof id === 'number' ) {
return id;
}

return null;
} ),
};
},
save( { attributes } ) {
const { images, columns = defaultColumnsNumber( attributes ), imageCrop, caption, linkTo } = attributes;

return (
<figure className={ `columns-${ columns } ${ imageCrop ? 'is-cropped' : '' }` }>
<ul className="blocks-gallery-grid">
{ images.map( ( image ) => {
let href;

switch ( linkTo ) {
case 'media':
href = image.fullUrl || image.url;
break;
case 'attachment':
href = image.link;
break;
}

const img = (
<img
src={ image.url }
alt={ image.alt }
data-id={ image.id }
data-full-url={ image.fullUrl }
data-link={ image.link }
className={ image.id ? `wp-image-${ image.id }` : null }
/>
);

return (
<li key={ image.id || image.url } className="blocks-gallery-item">
<figure>
{ href ? <a href={ href }>{ img }</a> : img }
{ ! RichText.isEmpty( image.caption ) && (
<RichText.Content tagName="figcaption" className="blocks-gallery-item__caption" value={ image.caption } />
) }
</figure>
</li>
);
} ) }
</ul>
{ ! RichText.isEmpty( caption ) && <RichText.Content tagName="figcaption" className="blocks-gallery-caption" value={ caption } /> }
</figure>
);
},
},
{
attributes: {
images: {
Expand Down