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

Cover: Allow drag n drop media replacement #29813

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

Mamaduka
Copy link
Member

Description

Adds dropzone for the cover block background image.

Thanks, @jasmussen, @paaljoachim for the initial mockups. Sorry if any metrics are off compared to mockups.

Fixes #26388.

How has this been tested?

Manually on local dev environment.

Steps:

  1. Add cover block.
  2. Upload a background image.
  3. Drag and drop replacement image.
  4. Dropzone indicator element should appear.
  5. See if the background image is replaced.

Screenshots

cover-image-dropzone

Types of changes

New feature.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

// Only target direct dropzone.
.wp-block-cover > .components-drop-zone {
&.is-active {
transition: 0.3s opacity, 0.3s border;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. We use 0.2s in most other places to make it snappy — if that feels okay to you, might be a good change. If it feels too snappy, okay to keep 0.3, but worth a shot.

Might want to include this right below the transition rule:

@include reduce-motion("transition");

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 think 0.2s looks better, and it isn't too snappy.

Wasn't aware of the reduce-motion mixin, Thanks for mentioning it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, it's easily missed. We have one for the animation property as well, and if you basically output it on the line right after either transition or animation both, you'll be sure that for people to check the system level setting have their needs met. It's a nice little trick, it just sets the duration to about zero.


&.is-dragging-over-element {
background-color: transparent;
border: $grid-unit-60 solid var(--wp-admin-theme-color);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice 👌

.components-drop-zone__content {
display: flex;
align-items: center;
top: -36px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should round this number up to 40px ($grid-unit-50), or down to 32 ($grid-unit-40). It's a small thing, but might be slightly more griddy. Alternately, I see the grid as being fundamentally base12, even though we leverage an 8px base. You could do ($grid-unit-15 * 3).

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested with both $grid-unit-40 and $grid-unit-40 initially, but 36px give it more "centered" look.

I've updated styles to use ($grid-unit-15 * 3).

@jasmussen
Copy link
Contributor

This is what I see:

this branch

This is stellar work. Thanks so much for the work. I left a few comments, but it's almost ready to land. It could use a small code sanity check (@aristath want to look at the JS bits?), but otherwise, sweet deal!

Copy link
Member

@aristath aristath left a comment

Choose a reason for hiding this comment

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

Nice! I Iike it 👍

@aristath want to look at the JS bits?

Code looks good to me ❤️
Tested and works as advertised.

@Mamaduka
Copy link
Member Author

Thanks, @jasmussen, @aristath

For taking the time and testing the PR.

I've pushed styling adjustments that @jasmussen suggested.

@jasmussen
Copy link
Contributor

🚢

@aristath aristath merged commit 877f615 into WordPress:trunk Mar 12, 2021
@github-actions github-actions bot added this to the Gutenberg 10.3 milestone Mar 12, 2021
@Mamaduka Mamaduka deleted the update/cover-dragndrop-replace branch March 12, 2021 12:13
@jasmussen
Copy link
Contributor

@Mamaduka It looks like we missed a small issue here for when you are using the Cover block without an image:

Screenshot 2021-03-14 at 09 13 21

Compare with the commit before this branch:

Screenshot 2021-03-14 at 09 13 57

Sorry about missing that. It's hopefully an easy fix. Want me to ticket it?

@Mamaduka
Copy link
Member Author

Hi @jasmussen,

Thanks for catching this regression, and sorry for missing it myself. I will create fixing PR ASAP.

@jasmussen
Copy link
Contributor

jasmussen commented Mar 14, 2021

Don't stress, take the weekend off!

@roo2
Copy link
Contributor

roo2 commented Apr 9, 2021

@Mamaduka, @jasmussen it looks like we're still seeing the issue with the misaligned paragraphs Automattic/wp-calypso#51670

I tested on latest trunk as well as whats on wpcom 👍

Screen Shot 2021-04-09 at 3 16 40 PM

@roo2
Copy link
Contributor

roo2 commented Apr 9, 2021

Sorry, it looks like the issue I was seeing is wpcom specific, caused by jetpack intercepting the drag-and-drop area introducd in this PR 🙏

@Mamaduka Mamaduka added [Type] Enhancement A suggestion for improvement. [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

I should be able to drag and drop an image as the Cover block's background
4 participants