Problem/Motivation
This issue is a subtask to implement the new design of the media library #3019150: Update/improve mockups and designs for the media library.
The current media library widget contains 2 buttons: 'Browse media' and 'Add media'. The 'Add media' button currently only allows file uploads. To allow oEmbed media (#2996029: Add oEmbed support to the media library) we need a separate form to allow URL input. While discussing the best way integrate this in the widget, we concluded adding more buttons to the widget is going to be confusing for users.
Proposed resolution
Make the following changes to the media library widget:
- Remove the current "Add media" button that opens the upload form.
- This will leave the widget with only one button, "Browse media", which the opens the media library. When #3023802: Show media add form directly on media type tab in media library is complete, the media library will display a small form to upload or add new media, right there, without needing an additional click. As part of that issue, the "Browse media" button will be renamed "Add media".
Screenshots
The patch in #4 changes to following:
Before
The "Browse media" button opens the modal with the media library, the "Add media" button opens the upload form.
After
The "Browse media" button has been renamed to "Add media" and opens the modal with the media library. The original "Add media" button has been removed.
Modal
The following modal will be opened. Adding new files can be done by clicking "Add media" in the modal. We will improve this in #3023802: Show media add form directly on media type tab in media library.
API Changes
This changes a concrete form widget plugin implementation (which is not explicitly part of the API). It removes the $addAccess property from the MediaLibraryWidget class, because there is no longer be a need to perform a separate access check for whether media can be added via the widget (it's all handled in the single, integrated media library screen, as per the designs). The code path which used $addAccess is dead, and since it is access-related, it is better to simply have a hard break than a soft deprecation, which could potentially result in confusion and, in the worst-case scenario, an access bypass.
Comment | File | Size | Author |
---|---|---|---|
#25 | 3020707-25.png | 78.44 KB | phenaproxima |
#8 | 3020707-before.png | 40.87 KB | seanB |
#7 | 3020707-modal.png | 840.19 KB | seanB |
#7 | 3020707-after.png | 33.81 KB | seanB |
#4 | interdiff-2-4.txt | 2.74 KB | seanB |
Comments
Comment #2
seanBHere is a patch to rename the 'Browse media' button to 'Add media', and remove the existing 'Add media' button that opened the upload form directly.
Comment #3
seanBComment #4
seanBNoticed the unused property and remove that as well.
Comment #5
phenaproximaI'm tagging this for product manager review, and here is my reasoning:
This patch doesn't stand alone. It's part of the suite of must-have issues listed at #2834729: [META] Roadmap to stabilize Media Library, and it doesn't really make sense without them. If this patch is committed before some of the other ones in that list, it will be, at least temporarily, a UI regression for people who are using the media library now.
That said, the media library is still experimental, and the UI is undergoing heavy refactoring, which means that we might be able to get away with this. But I don't feel like I can make that call one way or the other; only a product manager can do that.
Comment #6
seanBI agree. We split up the three issue below for easier review, but until all three are committed this issue adds an extra click for users uploading new files:
#3020707: Streamline buttons in the media library widget
#3020716: Add vertical tabs style menu to media library
#3023802: Show media add form directly on media type tab in media library
The alternative would be to combine the 3 issues, but that probably makes it harder for reviewers/committers.
Comment #7
seanBAttaching some screenshots.
Comment #8
seanBSomething went wrong?! Uploading the before screenshot again.
Comment #9
webchickThis patch leaves things in a sub-optimal state, since it moves the most common action (Add new media, e.g. images from your hard drive) behind an extra click. However, Sean has informed me that this is a temporary measure only until some of the other issues are completed, so that seems acceptable given the scope/size of the other changes, and given that the module is still experimental.
Just bear in mind code freeze is in only 2 months away at this point, so let's not let it linger like this for too long. :)
Comment #10
phenaproximaThe product manager has spoken! Let's get this done.
Let's also make sure that any CSS and JavaScript related to this button is gone. I think that's it before this is RTBC.
Comment #11
seanBJust did a double check but there does not seem to be specific CSS or JS for the removed button. I think we are good to go.
Comment #12
phenaproximaThen I think we're good here. RTBC! Time to put some rocket boosters on the next two issues...
Comment #13
phenaproximaComment #14
xjm@larowlan and I were just discussing the issue in light of @webchick's concerns in #9. Given that this is a beta experimental module (meaning UI changes are allowed), I think it's allowable to commit a temporary UX regression so long as a followup issue to resolve the regression is listed as a UX gate stable blocker on the module roadmap.
That said, one concern:
These are both internal API BC breaks. As a beta module, Media Library should ideally follow the deprecation best practice for internal APIs as well:
https://www.drupal.org/core/deprecation#internal
Is it sensible to provide some minimal BC for the property, and deprecate rather than removing? A
@depreated
on the property, a change to the constructor docs, making the parameter optional, and@trigger_error()
in the constructor (but still set the property) if it's passed, and... not exactly sure where this would be, but BC for when it was previously used, so that people checking the property's truthiness don't suddenly have their button disappear everywhere. ;)What do you think?
OTOH if we do decide to have the BC break, we might want a small CR. (Maybe we should have one regardless.)
If we deprecate, we can always revert this before stable if we need to. However, if we add the BC break, reverting it in 8.8.x would be disruptive again.
Comment #15
xjmAlso tagging to make sure the followup to fix the UX regression is added as a stable blocker.
Comment #16
xjmThinking more about
$addAccess
, this is potentially a situation where it's probably safer to have a deliberate break vs. an involved deprecation. It's referring to a UI element that essentially no longer exists, and it's also the sort of thing that could introduce an access bypass (if it's incorrectly evaluated asTRUE
) or major bug (if incorrectly evaluated asFALSE
).Sorry for noise; hadn't really thought about the issue thoroughly in #14. I have an automatic response to BC breaks in a diff. ;) But we should always document breaks and justify them (and include CRs when it changes how stuff works, which this does). The questions to ask every time are: Is it possible to provide BC? Are there reasons not to? What are the implications for someone extending this code if I do or don't? Let's document the answers here.
Comment #17
xjmHm, also. I see in the removed lines where the property was accessed, but not where it was ever set via a plugin manager or something. Where does/did that happen (and is there maybe more dead code related to it?)
Comment #18
phenaproximaIt's set in the static create() method. It piggybacks on the access of the MediaLibraryUploadForm class, which is the form displayed in the modal when you click the "Add media" button -- which is removed by this patch.
We don't need a dedicated follow-up for this issue; in accordance with the new designs in #3019150: Update/improve mockups and designs for the media library, we are permanently removing the "Add media" button from the field widget, and renaming the "Browse media" button to "Add media", which will pull up a modal that contains both the media library and the form to add new media items, integrated into the same screen. So this patch is, then, removing code that is completely dead in the new designs. As per @seanB's comment in #6, once #3023802: Show media add form directly on media type tab in media library is committed, you will be able to add new media items from the form widget with the same number of clicks as we currently have.
So, @xjm, I agree with your re-assessment of the situation: there is no point in having BC for the property, because it will make no sense with the new designs, it won't be used for anything, and the code path will not be executed anyway.
I'll post a change record and update the issue summary soon.
Comment #19
phenaproximaComment #20
phenaproximaFixed the IS.
Comment #21
phenaproximaChange record written: https://www.drupal.org/node/3024762
Comment #22
phenaproximaI think we have addressed all feedback here. Setting back to RTBC.
Comment #23
xjmThanks @phenaproxima. Are those two issues already listed as required UX gate stable blockers? That's the important consideration here. We're following the policy that the UX gate needs to be met to mark a module stable (but can be bypassed before that), so we need to make sure this piece of the UX gate we're skipping here will be formally documented on the roadmap. Otherwise we need to follow it here/.
I don't see the static
create()
method as something changed in the patch. Could you be more specific? The point is, why is there no corresponding change removing how it was set?CR looks good to me. Thanks! The same CR can be updated as the other issues land.
Comment #24
xjmOh, I would still also like to see an answer to "What are the implications for someone who was extending this widget?" We talked about the implications for core, but not for that.
Are we going to have BC breaks for them strung out across three issues?
Comment #25
phenaproximaIt is changed. I have attached a screenshot to show the full extent of that.
If someone was extending the widget, the formElement() method no longer returns a media_library_add_button sub-element. If they were relying on that specific element, it is no longer going to be there. I believe that that $addAccess was only used to influence that button's #access property. In other words, we are completely ripping out the media_library_add_button and everything related to it.
I'm not 100% sure. I can't make concrete promises right now, but I can say this: the next issue in this particular line of refactoring is concerned with adding new code in a new controller/service, so I don't think that will introduce any BC breaks. However, in the issue after that (in which we integrate the upload form with the main media library), I believe we will be entirely removing the media_library.upload route as well. It's potentially possible we could find a way to do that which preserves the route, or at least turns it into a redirect, and we can try to aim for that if needed. That's the biggest BC break I can foresee right now.
Comment #26
phenaproximaYes. They are near the top of the must-have list in #2834729: [META] Roadmap to stabilize Media Library -- which is an ordered list. I have also marked them as explicitly part of the UX gate.
Comment #27
xjmOkay, looks good to me, thanks @phenaproxima! And thanks for highlighting the part of the diff I was missing; had to read upstream code a bit and remember the entity magic to see how it was working. These are the key lines.
Doing one more code review before I commit this.
Comment #28
xjmI added @phenaproxima's words from #18 to the CR, because the point of CRs is to answer the "what impact does it have, and on whom?"
https://www.drupal.org/node/3024762/revisions/view/11252006/11252300
Comment #30
xjmAdding credit for @larowlan because he and I discussed this issue in Slack.
Comment #32
xjmAlright, reviewed the whole thing again and everything is in scope for the change. Committed to 8.7.x. Now it's time to get back to where we once belonged -- onward to the rest of the chain. ;)
Comment #33
xjmPublished the CR, and also added the two followup issues to the CR while I was at it.
Comment #34
seanBThanks @xjm! I'm on it :)