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 file block #15515

Closed
wants to merge 6 commits into from

Conversation

draganescu
Copy link
Contributor

@draganescu draganescu commented May 8, 2019

Description

Closes #14795
Also fixes #10521

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

55976214-53a8d700-5c94-11e9-98ee-a41235cc8466

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 works great!

file

Should get a code review before merge though. 👍

packages/core-data/README.md Outdated Show resolved Hide resolved
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.

Actually — I'm am seeing the issue @talldan reported in the old thread:

The filename and file link isn't updated when adding or replacing a file using 'Insert from URL' (could just use the url).

@gziolo gziolo added [Block] File Affects the File Block [Type] Enhancement A suggestion for improvement. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code labels May 10, 2019
@draganescu
Copy link
Contributor Author

@kjellr I have fixed the issue reported by @talldan and also rebased the PR

@kjellr
Copy link
Contributor

kjellr commented May 27, 2019

Thanks! This is looking great. The last issue I'm seeing is that the cancel button is missing for Contributor users:

Screen Shot 2019-05-27 at 11 48 46 AM

(I'm also thinking we should rewrite that description text depending on the user role, but I think that's probably another issue.)

@draganescu draganescu requested a review from ellatrix as a code owner May 28, 2019 20:05
@draganescu
Copy link
Contributor Author

@kjellr fixed this issue with no cancel link. This fix will also solve #15517 's similar problem because the issue is in the common mediaPlaceholder component, so if this gets merged first the other will display the cancel link too.

@mapk
Copy link
Contributor

mapk commented May 28, 2019

I just tested the file block and noticed a weird issue. I'm using FF.

Steps to reproduce:

  1. Add a file block.
  2. Select a file from the Media Library.
  3. After selecting the file, observe that the name is not replacing the placeholder text.

file

@draganescu
Copy link
Contributor Author

@mapk for me it doesn't happen in Firefox 67. What FF do you have?

@mapk
Copy link
Contributor

mapk commented Jun 10, 2019

@draganescu I just tested again using FF 67.0.1 and it's working fine. I'm not sure which version I had running when I saw that, but I know I just updated today before testing this out.

@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-file branch May 27, 2020 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] File Affects the File Block 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 Add a URL field for File blocks
5 participants