Media Library integration (includes introducing a new main content renderer/`_wrapper_format`)

Created on 12 June 2024, 7 months ago
Updated 18 September 2024, 3 months ago

Problem/Motivation

Integrate the Drupal Media directly with the Experience Builder, allowing users to upload, browse, and select images from the Media Library.

We need to consider at least following to be able to start working on this:

  1. How would a SDC author declare that a component can accept a media image? (Backend)
  2. How can we load the Media Library within the Experience Builder (which is a React SPA)? (Frontend)

Proposed resolution

Figure out how to make this very complex field widget (Drupal core's most complex one, by far) work. If it requires form alterations, semi-coupled theme engine changes, anything โ€ฆ it doesn't matter: make it work here ๐Ÿ‘

User interface changes

๐Ÿ“Œ Task
Status

Fixed

Component

Page builder

Created by

๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @lauriii
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    How would a SDC author declare that a component can accept a media image? (Backend)

    It should not declare such a thing. Component author must declare a slot, that's all. A slot can accept any renderable. Sending a media image to the slot is the responsibility of the component user, on the CMS side.

    How can we load the Media Library within the Experience Builder (which is a React SPA)? (Frontend)

    It was started as a React SPA, but will it stay like that? Maybe, one day, we will consider this choice has more drawbacks than advantages.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    How would a SDC author declare that a component can accept a media image? (Backend)

    I agree with @pdureau, sort of.

    A "media image" really means:

    • a Media entity
    • โ€ฆ of a bundle that is configured to use \Drupal\media\Plugin\media\Source\Image
    • Which corresponds to โ€ฆ an image.

    Step 1: SDC author defines the expected shape, and ideally using a predefined JSON schema definition

    So, the SDC author should not define what the UX is for getting an image in there. They should declare that they want an image. But there's (at least) 2 kinds of images:

    1. an URL to an image โ€” which could be sufficient if it gets used as a background image
    2. an URL to an image + associated metadata (alt + width + height), necessary to be used. in an <img> tag

    We haven't gotten very far with making all of these concepts well-defined at the SDC JSON schema level, but we do have some, and we do have it for the aforementioned 2:

    1. {$ref: 'json-schema-definitions://experience_builder.module/image-uri'}
    2. {$ref: 'json-schema-definitions://experience_builder.module/image'}

    (introduced in https://git.drupalcode.org/project/experience_builder/-/commit/f784af41, which is heavily inspired by @pdureau's https://git.drupalcode.org/project/ui_patterns/-/blob/28cf60dd776fb349d9...)

    Step 2: when opting in an SDC component to XB, the default value for each prop determines the field type

    ๐Ÿ“Œ "Developer-created components": mark which SDCs should be exposed in XB Fixed landed. One of the next steps is โœจ Allow specifying default props values when opting an SDC in for XB Fixed . An implied part of setting up a default value there is that a field type is selected at that time: whichever field type the default value for the StaticPropSource uses that the Site Builder configures, also determines what the field type is that Content Creators get to use (they do not get to choose: that'd make the UX unbearably complex).

    In other words: if the selected default value is some "media image", then the Content Creator will be presented with the ability to use a media image.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    How can we load the Media Library within the Experience Builder (which is a React SPA)? (Frontend)

    This question is insufficiently specific IMHO ๐Ÿ˜‡

    We've discussed before that as a first step, we intend to load the existing field widgets in the right-hand sidebar (presumably with their Form API-based definitions automatically rendered using https://www.drupal.org/project/jsx โ†’ , to bring real-time updates โ€” see ๐ŸŒฑ [META] Real-time preview: supporting back-end infrastructure Needs work ).

    But โ€ฆ the way that the question is phrased seems to refer to in-place editing. Which is not something we should be doing from the start. See #3453690-10: [META] Real-time preview: supporting back-end infrastructure โ†’ .

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    But โ€ฆ the way that the question is phrased seems to refer to in-place editing. Which is not something we should be doing from the start.

    Sorry for causing confusion in regards to the requirements. I did not intend to expand the scope to inline editing. #2 was intended to be specifically about how the field widget (displayed in the right drawer) should be opening the Media Library, and defining how the Media Library modal dialog should be rendered within the Experience Builder. IIRC, we did some initial research that it could be opened in an iframe, and was something that Mercury Editor and/or https://www.drupal.org/project/layout_builder_iframe_modal โ†’ were doing.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Ahhhh! So it's more focused on the "this ain't no simple single-document viewport, it's a viewport with many (i)frames" challenge. Now I get it ๐Ÿ‘

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    We haven't gotten very far with making all of these concepts well-defined at the SDC JSON schema level, but we do have some, and we do have it for the aforementioned 2:

    {$ref: 'json-schema-definitions://experience_builder.module/image-uri'}
    {$ref: 'json-schema-definitions://experience_builder.module/image'}

    Indeed, I was a bit "quick" pushing the opaque slot as the only solution. I still thing is by far the best, but a well-defined and structured object prop ("object" according to JSON, so an associative array, a Twig "mapping", not a PHP object) can also do the job, and the JSON schema reference system can help the author.

    However, let's not look at the contents of the URI string ("experience_builder", "image") to guess if it is an image or not. Those URI, like every URI, โ†’ are opaque too. An other reference URI (ex: {$ref: 'json-schema-definitions://whatever.module/amstragram'}) pulling the same JSON schema structure must be processed the same way.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States ctrladel North Carolina, USA

    2.How can we load the Media Library within the Experience Builder (which is a React SPA)? (Frontend)

    I've managed to decouple(ish) Entity Browser in the RJSF module, it still depends on some drupalSettings values so it's not fully decoupled. Never explored doing it for Media Library because Entity Browser did a good enough job for my needs and could be used for any entity type. Linking in case it's helpful. https://git.drupalcode.org/project/rjsf/-/tree/1.0.x/modules/rjsf_entity...

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    However, let's not look at the contents of the URI string ("experience_builder", "image") to guess if it is an image or not.

    ๐Ÿค” Why? This would result in broken components wherever images are used. Can you elaborate? ๐Ÿ™

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Linking in case it's helpful.

    Thanks!

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    ๐Ÿค” Why? This would result in broken components wherever images are used. Can you elaborate? ๐Ÿ™

    If {$ref: 'json-schema-definitions://experience_builder.module/image'} resolve to:

    {
      "type": "object",
      "properties": {
         "url": {"type": "string"; "format": "iri-reference"},
         "alt": {"type": "string"},
         "width": {"type": "integer"},
         "height": {"type": "integer"}
      }
    }

    The schema resolver will juste replace the reference to the resolved JSON schema snippet.

    Then, the logic (is it an image?) will apply on the resolved JSON schema.

    So, if a component author directly write (without using $ref):

    {
      "type": "object",
      "properties": {
         "url": {"type": "string"; "format": "iri-reference"},
         "alt": {"type": "string"},
         "width": {"type": "integer"},
         "height": {"type": "integer"}
      }
    }

    It will also works.

    Or if a component author just writes:

    {
      "type": "object",
      "properties": {
         "url": {"type": "string"; "format": "iri-reference"},
         "alt": {"type": "string"}
      }
    }

    It will also works.

    Or if a component author uses another reference resolving the same schema snippet.

    {$ref: 'json-schema-definitions://whatever.module/amstragram'})

    It will also work.

    It is solid architecture, using JSON schema reference as expected, without introducing "weird" behaviour by deducing information from the characters in that URL.

  • ๐Ÿ‡ฆ๐Ÿ‡บAustralia larowlan ๐Ÿ‡ฆ๐Ÿ‡บ๐Ÿ.au GMT+10

    At some point do we need to talk about formatters? Yes we might store a media reference - but we might format it using e.g. a different view mode or a responsive image format

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance pdureau Paris

    Integrate the Drupal Media directly with the Experience Builder, allowing users to upload, browse, and select images from the Media Library.

    When talking about Experience Builder, it seems we are not talking a lot about the blocks, layout, formatters... plugins. Are they still considered as first class object? Or are they becoming an afterthought? Do we plan to reimplement in Experience builder features already existing in Drupal (like Media Library Browser) instead of leveraging them?

    Experience Builder is currently looking like 2 different "products" bundled in one:

    • the initially promised easy-to-use, powerful & universal display builder, which will replace Layout Builders and others
    • SDC & design system / tokens integration / management / building. Ambitious, but still a bit blurry.

    I am worrying this "coupling":

    • will ask extra unexpected work, so we will not meet our ambitious projects targets (calendar, scope...)
    • will cut experience builder from large parts of the Drupal ecosystem by:
      • not exposing the SDC integration to the other tools
      • not leveraging the plugins provided by Druapl core & contrib
    • will create technical debt (when we will want to replace or revamp one of the 2 parts) and change management risk (if one of the 2 parts of Experience Builder is not welcomed by the community, it will take down the other part with it)

    Instead, let's restore the Drupal plugins we all know and love in the centre of the discussions and the architecture, as an interface between the display builder and the design system manager.

    What do you think about that?

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland
  • Status changed to Postponed 6 months ago
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland
  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I think we might be able to bypass the ๐Ÿ“Œ [PP-1] Add support for matching SDC prop shape: {type: string, enum: โ€ฆ} Postponed blocker by just going with manually defined first. Which is what ๐Ÿ“Œ Support complex SDC prop shapes: introduce (Storable)PropShape to compute field type storage settings Fixed would allow. I'll revisit this once that's done.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Status changed to Active 5 months ago
  • First commit to issue fork.
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar-pd

    omkar-pd โ†’ changed the visibility of the branch 3454173-media-library-widget-in-props-form to hidden.

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar-pd

    omkar-pd โ†’ changed the visibility of the branch 3454173-media-library-widget-in-props-form to active.

  • Merge request !154#3454173 Media library widget in props form โ†’ (Merged) created by bnjmnm
  • Assigned to bnjmnm
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    @bnjmnm Assigning to you because when we paired on this yesterday, you said you'd do a few more things that I don't see in the MR yet. I don't want our commits to cross. ๐Ÿ˜…

  • Issue was unassigned.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • Assigned to wim leers
  • Status changed to Needs work 5 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Assigned to bnjmnm
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I pushed things further.

    But before I dive very deep into the AJAX system, I want to make sure you're seeing the same error after clicking the button of MediaLibraryWidget. Because I'd swear it's different from what you showed me a few days ago ๐Ÿ˜…

    This is what I see:

    (note that url at the bottom, that's the one it opens)

    And the response to that request is simply

    {}
    

    I also ran into a JS error due to Olivero JS getting loaded โ€ฆ which somehow happens because of attaching core/drupal.ajax ๐Ÿคฏ Did you see that too? Any ideas?

    Back to you, @bnjmnm ๐Ÿค“ Will continue this on Monday!

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Closed ๐Ÿ› Component props edit form route should use a custom wrapper format Closed: duplicate as a duplicate.

    Updating this issue title because that would've been pretty much impossible for @larowlan to find based on the current issue title. (It happened to be relevant for this issue's scope.)

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    Found out what is causing the pile of olivero assets being added

    • core/drupal.ajax depends on core/drupal.message
    • Olivero extends core/drupal.message with olivero/drupal.message
    • olivero/drupal.message depends on olivero/messages
    • olivero/messages lists olivero/global-styling as a dependency, adding ~45 CSS files and the olivero/navigation-base library. Seems like a bit much for a messages library?

    The issue I'm running into when attempting to add media via the widget might be the same as the one in #31, but when I discussed it earlier I was showing where it fails server side - it is it looking for an 'image' field on the node entity vs what I'm guessing should instead be a property of a ComponentTreeItem?

    If press play in the debugger and let the PHP take its course the end result is the same as#31.

    It's not the slickest, but here's a few screenshots of the trace, starting with the most recent just-about-to-throw and a few steps back.

  • Assigned to wim leers
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Epic debugging trail there! ๐Ÿ˜ฌ

    I'll try to push this further today.

  • Assigned to bnjmnm
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    With the changes I just pushed up, the widget now works in the props form -> Short video if interested

    It does not yet update the layout when the media is updated, though, so that's the next step.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #35: thanks for that video, that's awesome! ๐Ÿ˜„

    FYI, I created a critical bug for a long-standing annoyance: ๐Ÿ“Œ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active .

    IMHO that belongs in the current sprint because it directly impacts the Barcelona demo. @lauriii confirmed.

    (Because \Drupal\Tests\experience_builder\Kernel\MediaLibraryHookStoragePropAlterTest::getExpectedStorablePropShapes() also contains the โ€ฆentity:fileโuriโžโŸvalueโ€ฆ prop expression, so without #3469436, itโ€™ll be impossible for @bnjmnm to get a selected image media to render correctly in the XB preview!)

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    @bnjmnm noticed that this also needed the fix that another front-end issue needed, so landed it in ๐Ÿ› Follow-up for #3467712: field instance settings lost in ComponentPropsForm Active , to help make this MR slightly simpler.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    The MR is messy as heck right now, but the widget now updates the layout: https://youtu.be/DFiE4r5mEFE, though as discussed previously it is passing the file URI so we're not seeing a new image there, but the alt text change and the prior image going away is pretty good evidence that it works.

    There's and extra step to get the existing image to show up in the media library widget when the props form opens, because it's not actually a media entity. Rather, it's an image field using the media library widget. If we want to do this, it will need to create a media entity for the image (perhaps on the fly) so it can be displayed within the widget. That should be easy enough, but I may ask around to be sure it's necessary.

    The MR probably has some junk in there from the different approaches I tried to make it work but I assure you it'll be less chaotic soon (but probably still little chaotic)

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Nice to see this working! ๐Ÿคฉ Do we already have an issue to load the media library using the admin theme? Or should we do that here?

    There's and extra step to get the existing image to show up in the media library widget when the props form opens, because it's not actually a media entity. Rather, it's an image field using the media library widget. If we want to do this, it will need to create a media entity for the image (perhaps on the fly) so it can be displayed within the widget. That should be easy enough, but I may ask around to be sure it's necessary.

    I don't think this is needed at least as part of this issue. We could give it some more thought and consider improving this in future.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #38: ๐Ÿฅณ๐Ÿš€ NICE! ๐Ÿ‘ (For the visual brokenness, @tedbow is working on that over at ๐Ÿ“Œ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active as I mentioned in #36.)

    Do we already have an issue to load the media library using the admin theme? Or should we do that here?

    Isn't that in direct contradiction of ๐Ÿ› Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme Fixed ? ๐Ÿ˜… Or are you reconsidering what you wrote at #3469686-4: Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme โ†’ , and we should use Claro instead? Or perhaps you're distinguishing between the form in the right sidebar (stark) and the media library dialog (claro)?

    Rather, it's an image field using the media library widget.

    ๐Ÿ˜ฎ That surprises me!

    That's not what media_library_storage_prop_shape_alter() specifies. Can you point me to where in the MR that is happening? ๐Ÿ™

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    Isn't that in direct contradiction of #3469686: Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme? ๐Ÿ˜… Or are you reconsidering what you wrote at #3469686-4: Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme, and we should use Claro instead? Or perhaps you're distinguishing between the form in the right sidebar (stark) and the media library dialog (claro)?

    I'm considering the right sidebar separate from this. The right sidebar is rendered using styles from the Experience Builder app โ€“ therefore it doesn't need the admin theme. In this use case, admin theme just adds an unnecessary layer where things can break without adding any benefit.

    The media library on the other hand is expected to be rendered just like how it's rendered in the admin UI. For this reason, I think it would make sense to render this using the admin theme instead of Stark (or some other theme). This might be an implementation detail but I think we should probably render the media library in an iframe or Shadow DOM to prevent styles from the XB UI leaking to the media library (and vice versa).

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #41: aha, great, so it's


    ๐Ÿ‘

  • Assigned to wim leers
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI

    So the widget works in the prop form and can very much update a component, but getting this process to work well in E2E proven a bit tricky.
    @Wim Leers said he'd have a look at this so assigning to him. Do a grep for @yo-wim and you'll find explanations next to their relevant lines of (sometimes commented) code.

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I was able to reproduce the same failure locally. ๐Ÿ‘

    Root cause found, it's a combination of:

    1. ๐Ÿ“Œ Create an Open API spec for the current mock HTTP responses Fixed having added an OpenAPI spec + a KernelEvents::RESPONSE subscriber to validate responses.
    2. We already knew that, and I updated this MR 20 days ago to make /openapi.yml aware of this API route.
    3. โ€ฆ but making the Media Library stuff actually work causes an AJAX request to be sent by Drupal's JS, which causes a POST request to that same route. ๐Ÿšจ That's where things go wrong: the openapi.yml file only had an entry for GET โ€ฆ

    Fix: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... โ€” E2E test is now passing ๐Ÿ‘

    ๐Ÿ’€ But why was this so excruciatingly hard to find? Well, because:

    1. \Drupal\experience_builder\EventSubscriber\ApiResponseValidator::validateResponse() throws an exception โ€ฆ
    2. โ€ฆ which then is not handled by anything else, resulting in the default AjaxResponse to be sent rather than the fully built one (which is done by \Drupal\Core\EventSubscriber\AjaxResponseSubscriber::onResponse()).
    3. Consequence: only the debug log message that appears at /admin/reports/dblog revealed the nature of the problem:
      </li>
        <li>OpenAPI spec contains no such operation [/xb-field-form/node/1]</li>
        <li>

    Creating critical follow-up issue for that โ€ฆ

  • Assigned to bnjmnm
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    And now the drupal_flush_all_caches() is gone, too!

    I think @bnjmnm can push this forward to finish the test coverage now ๐Ÿ˜Š โ€” I'll happily continue reviewing/pushing this forward tomorrow!

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    I didn't push it forward today because other issues required my attention. To be continued on Tuesday ๐Ÿ˜Š

  • Assigned to wim leers
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ
  • Assigned to bnjmnm
  • Status changed to RTBC 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    As far as I'm concerned, this is now ready. 12 open threads remain. I'd like @bnjmnm to sign off on those, and then this can/should IMHO land.

    Epic work here, @bnjmnm! ๐Ÿ‘

    (Next up after this: styling the Media Library โ€ฆ but to match what? Claro's Media Library/modal dialog styles?)

  • Issue was unassigned.
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States bnjmnm Ann Arbor, MI
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

    I just tried testing this and it didn't work:

    1. switched to mr branch
    2. cd ui
    3. npm i
    4. npm run build
    5. delete xb component config
    6. drush cr
    7. drag image component to desktop area
    8. click "choose file"
    9. error popup
  • Status changed to Needs work 4 months ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

    Moving back to needs work.

  • ๐Ÿ‡ซ๐Ÿ‡ฎFinland lauriii Finland

    @kristen pol is there an error in your sites error logs that you could provide here? That would probably help with debugging the problem.

  • ๐Ÿ‡บ๐Ÿ‡ธUnited States Kristen Pol Santa Cruz, CA, USA

    Just tested again and see 2 errors in the logs:

    TypeError: _semi_coupled_get_variable(): Return value must be of type object|array|string|bool|null, int returned in _semi_coupled_get_variable() (line 286 of /var/www/html/web/modules/contrib/experience_builder/themes/engines/semi_coupled/semi_coupled.engine).
    
    Drupal\Core\Render\Component\Exception\InvalidComponentException: [image] String value found, but an object is required in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of /var/www/html/web/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).
    
  • Status changed to RTBC 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #50:
    Did you see the message that @bnjmnm added? (Follow-up issue created to get rid of it: ๐Ÿ› Make Media Library widget work when JS aggregation is enabled Active .)

    It works fine here:

    (And zero warnings or exceptions being logged. If you find the precise STR for them, please create a new issue.)

    โš ๏ธ Note that the uploaded image does not show up because the public:// URI is being used rather than the corresponding computed http(s):// URL. @tedbow is working to fix that over at ๐Ÿ“Œ Fix the visually broken "image" component instance: use FileUriItem's computed `url` property, not the stored `value` property Active .

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Created ๐Ÿ“Œ Media Library dialog styling Active for #48.

    Unfortunately, I'm going to probably have to bypass review/approval from:

    • @traviscarden for the changes to /openapi.yml (and tiny refinement to ApiMessageValidator that removes the OpenAPI logged complaint about the /experience_builder.experience_builder route)
    • @hooroomoo or @effulgentsia for the changes to the semi-coupled theme engine โ€” while @bnjmnm has built most of that, sign-off from them would have been nice
    • @hooroomoo or @balintbrews or @jessebaker for the changes to /ui

    โ€ฆ because this issue is literally the #1 most important issue at ๐ŸŒฑ Milestone 0.1.0: Experience Builder Demo Active ๐Ÿ˜…

    I want to land this this AM. So I asked @jessebaker to review this in the next hour. Otherwise, I'll go ahead and merge this. It's too important progress to let get stale.

    (@traviscarden told me he was going to review last night but apparently he did not. @hooroomoo is out.)

  • Assigned to jessebaker
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Jesse's on it :)

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar-pd

    @kristen pol

    You'll get those errors if you don't have Media and Media Library modules installed.

    Here are the steps that I followed with a fresh Drupal installation.

    • Install Media and Media Library Modules.
    • Install XB.
    • Go to /admin/config/development/performance and uncheck Aggregate JavaScript files & save.
    • Add Article content.
    • Go to /xb/node/1 and test with image component.
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar-pd

    One more thing,

    If we upload an image with a space in its name(ex - image xyz.png) the following error is thrown.

    Drupal\Core\Render\Component\Exception\InvalidComponentException: [image.src] Invalid URL format in Drupal\Core\Theme\Component\ComponentValidator->validateProps() (line 203 of /var/www/html/web/core/lib/Drupal/Core/Theme/Component/ComponentValidator.php).

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    #58 That was fixed in ๐Ÿ› `{type: string, format: uri}` disallows image URLs containing spaces, `uri` data type stores + returns invalid URIs! Fixed โ€ฆ ๐Ÿค” Can you create a new issue for that problem and include the exact filename that triggers it? ๐Ÿ™

  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia omkar-pd

    Done @wim leers.

    Image src containing spaces throws error. ๐Ÿ› Image src containing spaces throws error. Active

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Unblocked @jessebaker on an environment issue, and that's how I realized that my GIF in #54 actually shows a problem: the "Add media" functionality in the right sidebar has Olivero styles โ€ฆ despite [#3469686} ๐Ÿ˜…

    Fortunately, it works fine with just Stark too (pushed a one-line commit to fix it):

    ๐Ÿ‘†that now defers all styling considerations to ๐Ÿ“Œ Media Library dialog styling Active , as intended ๐Ÿ‘

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom jessebaker

    I've just approved this from the /ui code perspective. There is obviously more to be done here, but I see no reason not to go ahead and merge this as a significant step forward.

  • Pipeline finished with Skipped
    4 months ago
    #273370
  • Issue was unassigned.
  • Status changed to Fixed 4 months ago
  • ๐Ÿ‡ง๐Ÿ‡ชBelgium wim leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    Great!

    Merged in upstream not once, but twice: first Felix' ๐Ÿ› Component config entities are incomplete: missing entries for optional props, causing errors in ComponentPropsForm Fixed , then Harumi's ๐Ÿ“Œ Improve the page hierarchy display Active !

    Still green CI, and per #55, this shouldn't wait any longer โ€” if the code owners disagree with the changes in the areas they did not approve, they are totally free to create follow-up issues ๐Ÿ‘๐Ÿ˜Š

    See y'all in ๐Ÿ“Œ Media Library dialog styling Active (which @lauriii prioritized for 0.1.0 โ€” see #3454094-36: Milestone 0.1.0: Experience Builder Demo โ†’ ).

  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024