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

Jump to bottom

Update gallery images to include post id #1469

Merged
merged 10 commits into from
Jun 30, 2017
Merged

Update gallery images to include post id #1469

merged 10 commits into from
Jun 30, 2017

Conversation

mkaz
Copy link
Member

@mkaz mkaz commented Jun 26, 2017

Fixes #1445

We need to send along to Media Modal the images with the post id, so when editing a gallery the dialog opens with already selected images.

Storing postId in image tag using data-postId to not confuse it with CSS id

@mkaz
Copy link
Member Author

mkaz commented Jun 26, 2017

An issue that still exists - inserting a Gallery from WordPress Core does not include the postId - so if a gallery is inserted with current Editor and then edited in Gutenberg, the images won't get loaded properly.

Likewise, how much of Gutenberg gallery code should match existing gallery code? We can duplicate it which should get Gutenberg as backward compatible, so a gallery created in Gutenberg shows as gallery in current Editor.

@mkaz mkaz requested a review from mtias June 26, 2017 21:36
mtias
mtias reviewed Jun 27, 2017
View reviewed changes
blocks/library/gallery/gallery-image.js Outdated
@@ -2,7 +2,7 @@
export default function GalleryImage( props ) {
return (
<figure className="blocks-gallery-image">
<img src={ props.img.url } alt={ props.img.alt } />
<img src={ props.img.url } alt={ props.img.alt } data-postId={ props.img.id } />
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like something we'd want as attributes in the comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Going to keep the attribute inline, since it adds complexity to include data in comment since would require duplicating the data set and then parsing out isn't

mtias
mtias reviewed Jun 27, 2017
View reviewed changes
blocks/test/fixtures/core-gallery.html Outdated
@@ -1,10 +1,10 @@
<!-- wp:core/gallery -->
<div class="blocks-gallery wp-block-gallery">
<figure class="blocks-gallery-image">
<img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" />
<img src="https://cldup.com/uuUqE_dXzy.jpg" alt="title" id="1" />
Copy link
Member

Choose a reason for hiding this comment

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

This would quickly run into conflicts, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed updating the fixture with data-postId, but looks like it should switch toan attribute in comment instead.

@mtias mtias added the [Feature] Blocks Overall functionality of blocks label Jun 27, 2017
@youknowriad
Copy link
Contributor

youknowriad commented Jun 27, 2017 •
edited
Loading

Side note: I updated the UploadMediaButton component to allow editing media passing a value prop. see #1459

@mkaz
Copy link
Member Author

mkaz commented Jun 27, 2017

@youknowriad I'll look at using that value prop for #1447 - the Gallery currently uses the gallery-edit modal dialog on editing, so doesn't use the Button but creates its own FormConfig. Ticket #1447 is asking for no extra controls and just select images, so those changes should help

@mkaz mkaz added the [Status] In Progress Tracking issues with work in progress label Jun 27, 2017
@mkaz
Copy link
Member Author

mkaz commented Jun 28, 2017

It turns out more than just id is needed for Media Modal to display gallery properly, after a bit of trial/error (thanks @georgeh) it needs the attributes [ 'sizes', 'mime', 'type', 'subtype', 'id', 'url', 'alt' ];

This PR switches the way attributes are stored to use comments, but slimmed down the set to store only those needed, not all that get returned.

To test - after creating a gallery save, and then close and reopen the post fresh. Testing it without refreshing or opening will include all the attributes the Media Modal sends back, not just what is stored. This was the error before why I thought it was working.

@mkaz mkaz removed the [Status] In Progress Tracking issues with work in progress label Jun 28, 2017
@mtias
Copy link
Member

mtias commented Jun 28, 2017

Why are all those needed? We don't want to use the media library sidebar panel for things like alt/url/size, etc. In fact we'd want to just disable that sidebar and use our inspector only.

@mkaz
Copy link
Member Author

mkaz commented Jun 28, 2017

@mtias ask backbone :-)

Just to get the images to load it required me to send those properties, I can try to use a different media modal dialog, but for the gallery-edit that is what is required. However, ticket #1444 also is asking for gallery modal on insert

mkaz added 10 commits June 30, 2017 10:39
@mkaz
Update unit test with post id
f9621cb
@mkaz
Update fixtures with data-postId
38f99a4
@mkaz
Update fixtures with gallery data-wp-media-id
82eebe3
@mkaz
Add slimImageObjects method to reduce images to just attributes we need
118db19
@mkaz
Remove hpq, no longer used in attributes
c104199
@mkaz
Be explicit with lodash require for pick
73242ae
@mkaz
Add updated fixtures for core gallery tests
44747ea
@mkaz
Add test for element.props
da19f15 
The unit tests fail if the element has a attribute called type, since that
triggers it thinking it is a React Tree.  Adds an additional check for
element.props which is assumed exists causing the error below.

```
  1) full post content fixture core-gallery:
     TypeError: Cannot read property 'children' of undefined
      at normalizeReactTree (build/test.js:8785:20)
      at build/test.js:8770:11
      at Array.map (native)
      at normalizeReactTree (build/test.js:8769:18)
      at build/test.js:8804:26
      at Array.map (native)
      at normalizeParsedBlocks (build/test.js:8795:16)
      at Context.<anonymous> (build/test.js:8816:33)
```
@mkaz
Simplify slimImageObjects method
5c535e2
@mkaz
Add missing core__gallery fixtures
538bbb9
@georgeh georgeh self-requested a review June 30, 2017 18:37
georgeh
georgeh approved these changes Jun 30, 2017
View reviewed changes
Copy link
Contributor

@georgeh georgeh left a comment

Choose a reason for hiding this comment

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

LGTM. I realize there's a bit of cruft to manage the gallery view in Backbone, hopefully that gets cleaned up and we can revisit that in the future.

@mkaz mkaz merged commit 7804758 into master Jun 30, 2017
@mkaz mkaz deleted the fix/1445-gallery-id branch June 30, 2017 18:48
@nylen
Copy link
Member

nylen commented Jul 5, 2017

The duplication of information between comment attributes and HTML markup introduced here looks quite bad, and is inviting a lot of problems related to one source falling out of sync with the other. We need to pick one method or the other of storing data.

@mkaz
Copy link
Member Author

mkaz commented Jul 5, 2017

@nylen open to suggestions on how not to duplicate the data?

@nylen
Copy link
Member

nylen commented Jul 24, 2017

Sorry, I have been meaning to look more closely at this but haven't had a chance yet. See also #1986.

@nylen nylen mentioned this pull request Jul 24, 2017
Closed
dratwas pushed a commit that referenced this pull request Nov 14, 2019
@marecar3
720-Add support for giphy and pexel images (#1469)
7411092
dratwas pushed a commit that referenced this pull request Nov 14, 2019
@daniloercoli
Merge branch 'develop' of https://github.com/wordpress-mobile/gutenbe…
245a642 
…rg-mobile into issue/AztecAndroid-IndexOutOfBoundsException-setSpan

* 'develop' of https://github.com/wordpress-mobile/gutenberg-mobile: (50 commits)
  Update issue templates
  Update gutenberg ref
  Update gutenberg ref
  Update issue templates
  Update GB ref to point to official 1.16.0 tag
  Update RELEASE-NOTES.txt
  Update JS bundles
  Update GB ref
  Update app bundles
  Update GB reference.
  [Aztec iOS]: `shouldInteractWith` will return always true to avoid crashes
  [Aztec iOS] Cleanup white spaces.
  Update bundles.
  Update version to 1.6.0
  Updated bundles
  Force translation update when generating new bundles
  Update gutenberg ref
  720-Add support for giphy and pexel images (#1469)
  Update bundles
  Update gutenberg ref
  ...

# Conflicts:
#	gutenberg
This was referenced Sep 16, 2021
Open
Open
@Verdinjoshua26 Verdinjoshua26 mentioned this pull request Jun 20, 2022
Open
@soloinovator soloinovator mentioned this pull request Jun 20, 2022
Open
@MaxMood96 MaxMood96 mentioned this pull request Jun 20, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtias mtias mtias left review comments

@georgeh georgeh georgeh approved these changes

Assignees
No one assigned
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mkaz @youknowriad @mtias @nylen @georgeh