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

Components: Add ImageSizeControl component #17148

Merged

Conversation

@ryelle
Copy link
Contributor

ryelle commented Aug 22, 2019

Pull out the image size controls from the image block into a standalone component. This also updates the Image block to use this new component. The new component allows for selecting the source image size (if provided), and then adjusting the size from there – so you can pick the thumbnail, which sets the image size as 150x150, and then you can change the dimensions from there. The image block fetches each image's data directly, so it sets the source image height & width based on the real image size.

The idea is that this now-shared component can be used by the latest posts block to control featured images (and any plugins that add images).

To test

  • Add an image block
  • Change the sizes
  • The preview and rendered frontend should all be updated as expected

There should be no functionality change here.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Aug 23, 2019

@jorgefilipecosta, is it something that would fit into “@wordpress/media-utils” package? I don’t think we want to add more components to the “@wordpress/components” package which aren’t general purpose. This one is highly coupled to the block editor so it would be great to find an alternative which will help to share components specific to blocks.

We discussed during the weekly Core JS chat that we should rather seek ways to make packages smaller. @nerrad, did you discuss it further this week? I guess we can still find some components which are very coupled to blocks in “@wordpress/components”.

@nerrad

This comment has been minimized.

Copy link
Contributor

nerrad commented Aug 23, 2019

did you discuss it further this week?

No, but for reference there's this issue I created (#17079) and during one of the GB triages this surfaced as well (#16709). I also just became aware of (#13910) as well which may help deal with some of the issues in some environments.

I agree I think there should be some effort towards evaluating what should go in @wordpress/components. However it's possible there isn't a "better" place yet and if so, shouldn't block maybe.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Aug 26, 2019

I agree I think there should be some effort towards evaluating what should go in @wordpress/components. However it's possible there isn't a "better" place yet and if so, shouldn't block maybe.

@mtias and @youknowriad, where are we with creating guidelines what should be part of @wordpress/components and how it should evolve? It looks like the recent audit performed by @drw158 made it clear that we should come up with a clear vision of how to approach all that, and adding new components should be the most important one.

@davewhitley

This comment has been minimized.

Copy link
Contributor

davewhitley commented Aug 26, 2019

It could be more explicit, but the guidelines require that the component be general use. It doesn't explain about different packages or a separate one for the editor though. I like the idea of having better organization for the packages, and a separate package for the editor, maybe @wordpress/components/editor.

Aside: I know it's new, but from now on the contribution guidelines should be followed when adding a new component.

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/CONTRIBUTING.md

@ryelle

This comment has been minimized.

Copy link
Contributor Author

ryelle commented Aug 28, 2019

@drw158 Are you asking me to make a change here? This isn't an entirely new interface, just component-ifying something that already exists, so the prototype/spec aspects of that process don't really apply here.

Overall, I'm happy to move this component wherever you all think is best, I personally have no input on where it belongs as long as it can be reused outside of core-gutenberg 🙂

@davewhitley

This comment has been minimized.

Copy link
Contributor

davewhitley commented Aug 30, 2019

I wish we had an editor package to make this easier. If the intent is to keep this a specific editor component, then it's probably fine as is. But, if the intention is to create a general use component for everyone, then it must go through the review process (figma component creation, QA, etc.). It doesn't matter to me if it's existing UI or not.

So to answer your question, no changes are requested from me, but it would be great if someone with the knowledge could make an editor package. Details and background laid out here #16709

@melchoyce

This comment has been minimized.

Copy link
Contributor

melchoyce commented Sep 5, 2019

What does this need to become mergeable?

@mapk mapk added the Needs Decision label Sep 5, 2019
Copy link
Member

jorgefilipecosta left a comment

@jorgefilipecosta, is it something that would fit into “@wordpress/media-utils” package? I don’t think we want to add more components to the “@wordpress/components” package which aren’t general purpose. This one is highly coupled to the block editor so it would be great to find an alternative which will help to share components specific to blocks.

I don't think media utils is a good fit for this component. Media utils contain media-related components/utils that are WordPress specific. Artifacts there need a WordPress instance with the wp global available (e.g: the component to abstract the wp media library, function to upload to the WordPress media library).

I think this seems like a pure component that does not depends on WordPress; it just contains select and input controls/buttons. It is a component that allows to input width and height and supports a set of predefined width, height options. So it seems a UI component like the others. If in the future, if we have an infrastructure that allows live preview components/samples having it in the components package would enable us to take advantage of this feature.
On the other side, I understand that although being a pure UI component, adding every pure UI in components may make the package too big.
Another good possibility is adding it to the block-editor components folder, it contains components useful for block developers and not necessarily generic components, e.g: media placeholder is there https://github.com/WordPress/gutenberg//blob/5ac0b123e61554032ce3387de28cccd5686fabea/packages/block-editor/src/components/media-placeholder/index.js, I guess this folder could be seen as the editor package @drw158 referred.

@jorgefilipecosta

This comment has been minimized.

Copy link
Member

jorgefilipecosta commented Dec 17, 2019

It seems we did not arrive at a consensus on the ideal place for the component. I guess as a way to move the PR forward we can move the components to the block-editor package and export the component with the __experimental prefix we have other components in a similar situation there.
What do you think of this option @ryelle?

@ryelle

This comment has been minimized.

Copy link
Contributor Author

ryelle commented Dec 17, 2019

Sure, I can update the PR in the next day or so to make that change 👍

ryelle added 7 commits Aug 21, 2019
If this isn't passed through, we don't show the dropdown
@ryelle ryelle force-pushed the ryelle:add/component-image-size-control branch from e8a699c to fbb187c Dec 29, 2019
@ryelle ryelle requested a review from ellatrix as a code owner Dec 29, 2019
@ryelle

This comment has been minimized.

Copy link
Contributor Author

ryelle commented Dec 29, 2019

@jorgefilipecosta I've updated the PR, so ImageSizeControl lives in @wordpress/block-editor now, and is exported as __experimentalImageSizeControl.

Copy link
Member

jorgefilipecosta left a comment

Thank you for the iterations @ryelle. LGTM 👍

@ryelle

This comment has been minimized.

Copy link
Contributor Author

ryelle commented Jan 7, 2020

Great! Does someone else need to approve, or can this be merged? I don't have access to merge, myself.

@youknowriad youknowriad merged commit 18393a3 into WordPress:master Jan 9, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Jan 9, 2020

@ryelle I see you have a bunch of good PRs already merged, I added you to the repo, so you could use branches, approve and merge PRs.

@ryelle ryelle deleted the ryelle:add/component-image-size-control branch Jan 9, 2020
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
jorgefilipecosta pushed a commit that referenced this pull request Feb 9, 2020
See #1594. This commits builds on #17148 to add featured image to the Latest Posts block. It uses that new ImageSizeControl control and adds an alignment control for image alignment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.