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

adds new edit flow to the video block #15516

Closed
wants to merge 3 commits into from

Conversation

draganescu
Copy link
Contributor

Description

Closes #14795

This is a follow up to the image block edit flow update which ports the new flow to all the blocks which have media with an edit state.

How has this been tested?

For now I've only tested locally.

Types of change

New feature (non-breaking change which adds functionality)

Screenshots

55976311-8783fc80-5c94-11e9-9262-1ac1cb55d890

Copy link
Contributor

@kjellr kjellr left a comment

Choose a reason for hiding this comment

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

From a design perspective, this is all set!

video

Should get a code review before merge. 👍

@gziolo gziolo added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label May 10, 2019
Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I left some minor comments. The code seems ready after the minor changes are addressed 👍

packages/block-library/src/video/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/video/edit.js Outdated Show resolved Hide resolved
@draganescu
Copy link
Contributor Author

@jorgefilipecosta I have updated the code as per review and also rebased. Let me know if I understood your comments properly :)

@mapk
Copy link
Contributor

mapk commented May 28, 2019

I'm not sure if this is solely related to the Video block because I haven't tested it with others yet, but there's a bug when transforming the Video block to a Media+Text block and then back again to a VIdeo block. The Edit icon changes position in the block toolbar. This shouldn't happen.

Expected behavior:

Icons in the toolbar should remain in the same position for each block.

video

@draganescu
Copy link
Contributor Author

@mapk see this comment on the same issue on the embed block.

I will treat it in a separate issue.

@afercia afercia added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jun 7, 2019
@afercia
Copy link
Contributor

afercia commented Jun 7, 2019

Related to the "swapping" icon: #16048

@draganescu
Copy link
Contributor Author

#11952 changed direction so closing this as it became irrelevant.

@draganescu draganescu closed this Jul 31, 2019
@youknowriad youknowriad deleted the update/new-edit-flow-video branch May 27, 2020 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Video Affects the Video Block [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand the new replace image flow out to other blocks
6 participants