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

Reinstate OEmbed and hotlinking in "Insert Media" dialog #6480

Closed
3 tasks done
chillu opened this issue Jan 11, 2017 · 31 comments
Closed
3 tasks done

Reinstate OEmbed and hotlinking in "Insert Media" dialog #6480

chillu opened this issue Jan 11, 2017 · 31 comments

Comments

@chillu
Copy link
Member

chillu commented Jan 11, 2017

We've replaced the "Insert Media" dialog triggered by HTMLEditorField with a mini-version of the new React-based AssetAdmin. Which doesn't have a UI for triggering "Insert from URL", making it impossible to use OEmbed to insert youtube videos etc, as well as hotlinking to images.

Note: If the asset-admin module isn't installed, it falls back to the old HTMLEditorField dialog.

@clarkepaul It doesn't look like this has been considered in the designs? https://projects.invisionapp.com/boards/9GX68PTXUQFH#/1547737/71595080

===

Pull requests

@clarkepaul
Copy link
Contributor

clarkepaul commented Jan 11, 2017

"Insert from URL" will be a link from TinyMCE directly to a modal (not asset-admin) as the items don't live in the files area.
For example replace the "Media" link with "Add via URL"
image

@chillu
Copy link
Member Author

chillu commented Jan 11, 2017

So we're adding the toolbar to our TinyMCE? We still need to implement "insert from URL", regardless if its triggered from an icon or a toolbar. It's very custom functionality to SilverStripe (OEmbed logic for youtube, vimeo, etc), we can't rely on standard TinyMCE stuff here.

@clarkepaul
Copy link
Contributor

Not everything pictured there (thats just a screenshot from the TinyMCE site), but yes an insert dropdown was my solution. When we upgraded TinyMCE to 4 I made some recommendations/guides for how it should be set up to make sure it was responsive enough and had the right functionality but unfortunately those didn't make it into the merge. I was pretty sure there was an issue for "insert media by url" but can't find it.

We can remove the old asset-admin tabs "from CMS" and "from your computer".

FYI we need to start using dropdowns in TinyMCE if we are going to make the editor usable on smaller screens.

@chillu
Copy link
Member Author

chillu commented Jan 11, 2017

@clarkepaul TinyMCE configuration aside, this is a feature which needs to be designed and developed right? I can't see any reference to followup issues in the original insert media via react ticket. Only foun one for improvements to insert link UX, but that's separate. Can you confirm that we don't have design for "insert media by url" yet?

@clarkepaul
Copy link
Contributor

There is no design, it wasn't part of the previous set of work. Last I heard it was acknowledged that it needed to be done at some stage but not as part of the alphas—I took that as meaning it wouldn't become a React component and left with the current functionality with the other tabs removed.

@tractorcow
Copy link
Contributor

+1 on making "insert from url" a separate modal / dialog. It doesn't need to be a part of the same asset-admin react dialog. It could easily be a button, if we don't go the route of adding the menu bar.

@chillu
Copy link
Member Author

chillu commented Jan 12, 2017

Current view

image

image

@flamerohr flamerohr modified the milestones: CMS 4.0.0-beta1, CMS 4.0.0-alpha5 Feb 26, 2017
@clarkepaul
Copy link
Contributor

We need to add an additional action on TinyMCE to insert media, as a temporary measure we could add just a single action but seeing as we need additional space for smaller screens, and there are a lot of modules which add complexity to the TinyMCE UI a dropdown to hold "Insert" type things makes sense. I realise this issue is purely adding the "Insert media" dialog so some of this functionality within the mockups should be separated out (eg. moving the "Insert from word" and "insert as plain text" to the dropdown).
image
Same image on CMS UI visual style guide https://projects.invisionapp.com/boards/HVX68PZCFE43/

Design for actual dialog still to come...

@clarkepaul
Copy link
Contributor

clarkepaul commented Feb 27, 2017

@flamerohr I would like to add the ability to edit the details of media without having to select media/image buttons (as it isn't obvious how to edit media details). Do you think it would be possible to add an "Edit" link on top the media when hovered or selected? e.g.
pasted_image_27_02_17__1_15_pm

@flamerohr
Copy link
Contributor

@clarkepaul certainly possible, (for future ref: http://archive.tinymce.com/wiki.php/TinyMCE3x:Command_identifiers )
Although it will mean adding markup (unreliably) to the user's content (and then removing before save).
The UX is a nice idea, I do see this potentially becoming a maintenance issue, where it's not meant to be sent back to the server like shortcodes are.

@clarkepaul
Copy link
Contributor

clarkepaul commented Feb 27, 2017

Added more designs to styleguide here https://projects.invisionapp.com/boards/9GX68PTXUQFH/

image

@clarkepaul
Copy link
Contributor

I will supply placement images when you require them.

@flamerohr
Copy link
Contributor

@clarkepaul Got to review the designs properly. From the looks of it, will we disable editing the link when it's established?
Instead the Oembedded link will need to be recreated?

@flamerohr flamerohr self-assigned this Mar 6, 2017
@clarkepaul
Copy link
Contributor

Yup, you can't edit the url once entered—although it would be good to keep a way of editing the meta info of the embed. That is why I was asking about having a edit button in one of the corners when in focus. The old way was to select the embed and then select the image button which I thought could be weird as that is not the action for adding the media anymore.

@flamerohr
Copy link
Contributor

flamerohr commented Mar 13, 2017

@clarkepaul A few tweaks to the mocks will be needed to accomodate with FormBuilder limitations (might be able to achieve these later)

  • Split the "Add media" button to be below the text box

I think the rest are achievable, the placement fields will be css manipulated.

@clarkepaul
Copy link
Contributor

With CSS you can use .btn-group (Bootstrap) to join them but I take it its more about the js or form templates. Not a bit issue.

@chillu
Copy link
Member Author

chillu commented Mar 14, 2017

@clarkepaul How about aspect ratio on dimensions? Same issue as with "insert image", we don't want users to crank out the calculator.

@flamerohr I assume there's a loading spinner while SS goes and fetches the OEmbed data? Does the URL input become disabled while that happens? Otherwise you can get into edge cases were multiple concurrent requests happen out of sync

@clarkepaul
Copy link
Contributor

@chillu With the files area it used to auto tune the aspect ratio of images when one of the dimensions is modified (I would hope the new files area still has that functionality). I would expect the same pattern to be used here for media dimensions. If we allow users to override the aspect ratio then I would think thats additional functionality and possibly a new issue.

@flamerohr
Copy link
Contributor

@chillu there's a separate story for aspect ratio silverstripe/silverstripe-asset-admin#308
There does need to be a little bit of thought put into the UX of the dimensions component, e.g. button to lock ratio or release it.

The form disappears while SS goes to fetch the OEmbed data by virtue/sin of FormBuilderLoader using a different schemaUrl, so users won't be able to have another request made anyway.
To show loading could be a story for the FormBuilder, as opposed to part of this story, maybe?

@sminnee sminnee added this to the CMS 4.0.0-alpha6 milestone Mar 20, 2017
@flamerohr
Copy link
Contributor

@flamerohr flamerohr assigned flamerohr and unassigned flamerohr Mar 21, 2017
@flamerohr
Copy link
Contributor

I've updated the styles of the placement icons (and added text that are translatable)
Should be closer to the mocks, the text ellipsis if it overflows the width of the icon, rather than wrap to a new line.
screen shot 2017-03-23 at 10 47 05 am

@flamerohr flamerohr removed their assignment Mar 23, 2017
@clarkepaul
Copy link
Contributor

Looking good @flamerohr :)

@clarkepaul
Copy link
Contributor

This issue is more about the actions with the tabs within the edit panel (save and publish actions)

Opt 1. Only show main actions on the "Details" tab:
pasted_image_23_03_17__1_30_pm

Opt 2. Fixed actions:
pasted_image_23_03_17__1_28_pm
After some thought I think going with opt 1 would be the best approach for the time being as there will potentially be more actions on the history panel which could conflict with the main actions here.
. @flamerohr whats your thought on this?

@flamerohr
Copy link
Contributor

Oh ok, I think opt 2 was the easiest to implement (via the .form--fill-height class)
opt 1 would require some wrangling with the way fields and actions are built by the FormBuilder... Might even be juggling/refactoring the schema to be structured differently.

I agree that opt 1 would be the best approach, it would require more time to address though.

@clarkepaul
Copy link
Contributor

Cool, opt 1 it is even though it will be a bit more wrangling.

@clarkepaul
Copy link
Contributor

clarkepaul commented Mar 24, 2017

@flamerohr it is quite different from the designs.
Things for initial view:

Loading second view:
The modal seemed to collapse completely while it was loading the content, is there a way to avoid this collapse otherwise we need to add a loading icon to hold the height.

Second screen:

  • Header should read "Insert" not "Edit" when inserting and "Edit" when editing media which is already inserted (primary action should change too "Insert" to "Update"). Does this title need to be consistent when editing (editing media already inserted) to make the same header and the insert modal?
  • Would be good to increase the size of the file title
  • Can URL go as per design?

Side issues (not related to this issue):

  • Modal header is too small

@flamerohr
Copy link
Contributor

@clarkepaul thanks for the feedback

A few points:
Using input groups: We're unable to join the input and action together due to the way FormBuilder and schema works. It'll require a custom field to be built, but then FormBuilder currently does not support having FormActions within the Fields list (currently treated as separate grouped entities)

The title for the second screen would need to be consistent and the same for inserting a new or editing an existing, it's indistinguishable within the code.

I'll look at implementing the rest of the points :)

@clarkepaul
Copy link
Contributor

@flamerohr we can change the title to "Media from the web" so it is consistent for both use cases.

@flamerohr
Copy link
Contributor

@clarkepaul @tractorcow thanks for the reviews, changes have been made according to feedback

@flamerohr
Copy link
Contributor

All merged :D

@clarkepaul
Copy link
Contributor

Nice work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants