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

Blocks: FeaturedImage component #36

Closed
4 tasks done
coreymckrill opened this issue Mar 22, 2019 · 5 comments
Closed
4 tasks done

Blocks: FeaturedImage component #36

coreymckrill opened this issue Mar 22, 2019 · 5 comments
Assignees
@vedanshujain
Milestone
Blocks v1

Comments

@coreymckrill
Copy link
Contributor

coreymckrill commented Mar 22, 2019 •
edited by vedanshujain
Loading

Here are remaining issues I see with this component, including some based on unresolved discussions from #33:

  • The options for the size dropdown are still hard-coded and seemingly arbitrary. There are a few unresolved discussions that mention this (#, #, #, #). I think we should either populate with the site settings for media sizes, or consider switching the size controls over to the same UI used for avatars (see AvatarSize component).
  • Images shouldn't be scaled up beyond their actual dimensions. If the width option is set to 600 but the full size of an image (via details from the REST API) is only 394, the width attribute for that image should be 394, not 600.
  • The HTML markup, especially class names, should be consistent between the front end and the back end. (Edit: Being fixed in Miscellanous blocks fixes #66 )
  • Consider moving render_featured_image to includes/shared.php (#) (Edit: Being fixed in Miscellanous blocks fixes #66 )
@coreymckrill
Copy link
Contributor Author

The more I think about it, the more I think we should use the same UI for the size that we do for avatars. What do you think of that, @vedanshujain? Maybe we could abstract the AvatarSize component and rename it so that it could work for both scenarios.

That would only affect the stuff in the Inspector panel, not the actual FeaturedImage component.

@vedanshujain vedanshujain mentioned this issue Mar 25, 2019
Closed
2 tasks
@iandunn iandunn added this to the Blocks v1 milestone Mar 29, 2019
vedanshujain added a commit that referenced this issue Apr 4, 2019
@vedanshujain
WC Blocks: Change output markup to suppot links for consistency
4a7b9c2 
We already support linking featured image in editor. This patch adds support for linked image in rendered post as well. Also markup is changed a bit (especially classnames) to make both editor and rendered post consistent.

Towards #36
@vedanshujain
Copy link
Contributor

Images shouldn't be scaled up beyond their actual dimensions. If the width option is set to 600 but the full size of an image (via details from the REST API) is only 394, the width attribute for that image should be 394, not 600.

I wonder if that's a good idea, because it seems to go against WYSIWYG.

@coreymckrill
Copy link
Contributor Author

because it seems to go against WYSIWYG

🤔 I guess either way, the image doesn't look great in the context of the other featured images that do have the right width and resolution. @melchoyce any thoughts here on expected behavior?

To clarify, if we're setting the width of the featured images for a group of sponsor posts, should images that have a lower original width than our setting be displayed at their maximum width, or at the width we set, and be pixelated?

@melchoyce
Copy link

Left a note about this on #62.

vedanshujain added a commit that referenced this issue Apr 5, 2019
@vedanshujain
WC Blocks: Change output markup to suppot links for consistency
f000d2f 
We already support linking featured image in editor. This patch adds support for linked image in rendered post as well. Also markup is changed a bit (especially classnames) to make both editor and rendered post consistent.

Towards #36
vedanshujain added a commit that referenced this issue Apr 5, 2019
@vedanshujain
WC Blocks: Change output markup to suppot links for consistency
fc5c1ae 
We already support linking featured image in editor. This patch adds support for linked image in rendered post as well. Also markup is changed a bit (especially classnames) to make both editor and rendered post consistent.

Towards #36

git-svn-id: https://meta.svn.wordpress.org/sites/trunk/wordcamp.org@8599 74240141-8908-4e6f-9713-ba540dce6ec7
@vedanshujain
Copy link
Contributor

Most of this is fixed. Some discussion in #62 . Closing this one.

@coreymckrill coreymckrill mentioned this issue Apr 11, 2019
Closed
@coreymckrill coreymckrill mentioned this issue May 9, 2019
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@vedanshujain vedanshujain

Labels
None yet
Projects
None yet
Milestone
Blocks v1
Development

No branches or pull requests
4 participants
@iandunn @coreymckrill @melchoyce @vedanshujain