Just to clarify, what was spotted in #11 was mentioned in the summary.
I feel it's important to clarify it is something that props-form-triggered media library widgets have never been capable of doing, and not an issue introduced by any of the theming efforts, nor should it be within the scope of those. The need to get this working should be reflected in the issue queue, of course, but (at the risk of nitpicking) it's not a followup to this issue - it's just one of many things that need to happen as part of constructing Experience Builder.
But yep lets be sure it is documented and findable in the issue queue π Media Library dialogs triggered from page data do not have buttons yet Active
bnjmnm β created an issue.
Tests added in π Media library e2e should include multiple media replacements Active
bnjmnm β created an issue.
The "add media" button can get a little flaky if it is used multiple times, not related to the changes in this issue. There's an NR merge request that addresses that here: π Image can only be replaced one time before "add media" stops working Active
The e2e tests need to be refactored because the less aggressive variable replacement means I cant use CSSStyleSheets
the same way in the test. This can be reviewed without waiting on those tests, though.
The issue spotted in #9 made it apparent that I needed some additional changes for it to work right with aggregation, which is fixed in d7e0ea26
The trickiness is due to single variables being assigned new values in the same css file that then uses those variables. It's no problem when they are separate files, but when aggregated together there needs to be additional logic to ensure the dialog scoped version manages the variable values correctly.
Thanks @hooroomoo! looks like #9 happens with aggregation on. Time to dig further.
Discussed with @lauriii that it would be OK to provide a non-disruptive fix that includes an arbitrary wait, and we can seek out the robust version in a followup: π Replace arbitrary wait in attachBehaviorsAfterAjaxing with an occurence based one Active
Note the test failures in the pipeline are unrelated to the changes in this issue. You can see PHPUnit passing on the previous major
runs
We discovered #7 was due to triggering the media library from somewhere other than the article props form, which is currently the only supported way to do this, but will soon be available to other uses π Exapand Media Library admin theme rendering beyond props form. Active (that issue is ready, the test fails are unrelated to the MR)
bnjmnm β created an issue.
XB Page - Page Data (no buttons - as was the case pre-#3471978)
Article - Page Data (no buttons - as was the case pre-#3471978)
XB Page - Props (the "add media" button does not work reliably, which is not in this issue's scope. It was the case pre-#3471978 and will likely be fixed with the solution reported in
π
Image can only be replaced one time before "add media" stops working
Active
Or, do you feel this needs no further review, @bnjmnm?
There should at least be one additional person manually confirming Gin seems good and Claro didn't regress. There's still the drop shadow mystery I need to figure out too, but could see the benefit of getting the significant improvements provided here merged (it goes from unusable to completely usable) and the fine tuning can happen with a bit less urgency and perhaps a good newer-contributor issue.
@niranjan_panem that is correct because, as the issue summary states Claro's file-link template always displays file_size
The template is where the issue lies
The preprocess in the issue summary was explaining how other themes addressed this in the past.
Gin targeted refinements are happening here π Dialog + Media Library refinements to support Gin Active
Most recent changes to the MR addresses the buttonpane weirdness. The drop shadow still isn't getting in for some reason - I still need to figure that out - but I'm going to switch to NR so interested parties can start looking at it.
I have this addressed in π Dialog + Media Library refinements to support Gin Active but I'll hold of on closing this issue just in case that one stagnates for some reason
I uploaded a patch β that Gin will need to make it work (and will file an issue for it - the closet issue is filed in Gin toolbar and not gin)
Most of the support can be taken care of on the Experience Builder end, and the MR is full of that
Experience Builder
Regular Gin
bnjmnm β created an issue.
bnjmnm β made their first commit to this issueβs fork.
bnjmnm β created an issue.
I found an additional problem and created π add_css override should support var replacement for variables declared within the same file Active for it- this is something we would have only run into if aggregation was enabled - and I'm guessing most of us have it turned off. The symptoms are fairly mild: visual differences that don't impact functionality, but the fix shouldn't be hard and the IS documents the underlying cause.
I wouldn't be surprised if there are a few more of these. This is a huge change, and it's among the reasons I documented the heck out of it π€
bnjmnm β created an issue.
bnjmnm β created an issue.
#97 is what it would look like if \Drupal\experience_builder\Theme\XBThemeNegotiator
failed to see and/or process the 'use_admin_theme'
query arg in the AJAX request, and loads with xb_stark instead. Any time I've run into this it is due to the cache being out of sync with the current branch and a cache clear took care of it all but a few times - and that was fixed by truncating the cache tables. I've never run into the issue on a new install.
#98 This functionality targets the media library widget within the props form, which was all that existed when these efforts began. It can certainly be expanded to cover other uses in a separate issue.
bnjmnm β changed the visibility of the branch 3471978-try-using-admin-theme-without-fence to hidden.
The problem I spotted in #92 does not happen on a fresh install. I'm guessing it's legacy junk but if it happens to occur again I can probably knock it out more effectively in a separate issue anyway.
πΏπΏπΏ
Recording the outcome for a nice gif and I'm running into Claro base assets being added to the whole page instead of just the dialog. This means the "add button" style will look like xb_stark at first, then the dialog is opened and then it looks like Claro. I'm guessing this is due to the recent change of making the admin theme base styles a dependency of the Media Library instead of adding it as part of the dialog open request.
bnjmnm β created an issue.
Given that I've had to reopen several issues incorrectly closed by @niranjan_panem and
I tested the issue, views-pages and exposed forms are working fine in drupal, but not able to reproduce the issue.
Doesn't provide enough detail to indicate the steps to reproduce were followed, this may not have received the due diligence required to justify setting this to Closed (cannot reproduce)
I was just able to reproduce it in 11.x using the steps described in #12
@niranjan_panem I have reopened multiple legitimate issues today that you had closed due to incorrectly identifying them as no longer occurring. This hurts the project. If you'd like some mentorship regarding how to better do this, please join Drupal Slack (I could not find you there otherwise I would have pinged this to you there), and you can get some mentoring on this.
dfaf6b18 takes care of the alt validation message. The form generated by setEntityFields
was not taking access into account so it was validating fields that should be ignored as their access is false.
#36 Given the age of the issue, this may very well be outdated, but the rationale given is not valid. Drupal core has never had a "button field" so there not being one would not be evidence of this not being an issue. This is about how buttons are styled. Buttons have been and continue to be part of Drupal, but they aren't a field type.
#4 confirmed the presence of file-audio.html.twig
& file-video.html.twig
, but the issue is reporting on field-audio.html.twig
& field-video.html.twig
(note they begin with field, not file).
However, these files don't necessarily even need to be present, The specific concern is that Audio and Video field formatter classes are not called. This can be manually tested by going to the formatter settings for a video field and confirm changes to height/width/muted are reflected in the rendered page.
#82 and #83 were specific to my dev environment.
MR feedback is addressed. Please note there that there a new intermittent fail in the media library E2E test on 0.x. If a reviewer is seeing a Cypress fail that matches what I reported in π Intermittent fail in media-library.cy.js Active , it is unrelated to the changes in this issue.
bnjmnm β created an issue.
Sometimes I'll get an error from the dummy props form, but it isn't consistent. Here's an example of what I might run into after clearing a cache and accessing a path with a component selected... It expects a JSON response and instead gets the contents of core/modules/ckeditor5/css/table.css
. I'm pretty confused by this ATM, but I'm sure there's an entertaining answer to all this π€
After merging in the current 0.x, it's not possible to click any component (no image/media library needed) and get the props form . It just opens the panel but no form appears.
This works fine on 0x, so clearly there is something in this MR causing the problem. It's kind of difficult to isolate changes but I'll dig around and the DEBUG-3471978-try-using-admin-theme-without-fence
branch is with the current 0.x - I didn't update the main MR yet with it.
A few small things to address in the MR. Should be able to RTBC when those are done.
wim leers β credited bnjmnm β .
bnjmnm β made their first commit to this issueβs fork.
Excellent feedback from @longwave has been addressed.
bnjmnm β made their first commit to this issueβs fork.
re #20
I'd like to make it @larowlan's top priority in the next 2 weeks to keep pushing forward on #3467954: META: Evolve XB UI's data model to allow non-SDC components' inputs, DynamicPropSource support, etc. and all its child issues/blockers to solve this at the root.
That issue seems like the right priority, When that work is done, would it still be necessary for every prop-example image to become a media entity? It's fine with our dev-collection of components, but as more custom components start happening I could see that causing issues, such as someone deleting that placeholder image would make the component un-addable. It might be helpful to have a placeholder image generating controller, similar to placehold.it, where the URL determines width, height, color, content, then the required src
for preview won't be at risk of deletion and repos don't need to have as many image files just to placehold stuff.
Again, the data model work seems like the priority to me, but if the above seems worthwhile this issue could be the home for it as it would make the band-aid unnecessary.
bnjmnm β made their first commit to this issueβs fork.
bnjmnm β created an issue.
I got the go-ahead to use @scope
by @lauriii, but I now realize that was done outside of this issue so that's bad messaging on my part. I also had @scope
support enabled in FF long enough that I'd forgotten it's not yet available by default.
I could go with not scoping at all or, it's reasonably easy to fall back to regular selectors when @scope
isn't selected and in most cases it should work fine but there might be a few specificity hiccups that will go away as soon as FF switches scope from experimental to default.
It can be done with a ref!
Updated MR on the way...
Screenshot from 10.4, does not seem to be a problem for me either
Perhaps the original reporter can specify what element / which style is being set to a higher Z-index that results in the existing .media-library-item__remove
Z-index setting not working.
Added π Media Library item styles assume contextual module is present Active for the issue spotted in #72.1 where Claro expects contextual.module for styling media library items.
bnjmnm β created an issue.
Yep its good
#73.1
There's still something off about the grid items because the label is appearing on top of the border
Outside of XB, the label positioning relies on this style from contextual.module.css
.contextual-region {
position: relative;
}
contextual/drupal.contextual-links
isn't anywhere in the media library dependency chain, so that stylesheet isn't guaranteed to load, just very likely to be as pages it is used on tend to have that library present anyway. Seems like there should be a core issue since the appearance depends on an asset that is not explicitly summoned.
#73.2+.3
This throbber should be using the full screen throbber styles:
looks like the drupal.ajax library also needed to account for the admin theme's overrides and extensions. A side benefit of this is we no longer need the workaround that was implemented to avoid all the extra Olivero assets from π Many css files incorrectly being loaded on /xb/ pages Active
#73.4
After adding media and I click "Remove", it opens Media Library with broken styles:
The remove buttons needed similar ajax property customization to what was provided to the dialog open button.
It is working fine for me with what the install hook provides.
To narrow down what might be different it's probably worth checking admin/content/media to see if the media entity created via the install hook actually got created, and if so, is it accessing the image properly
There are two steps in the process that can be checked.
First Step
First, the image in experience_builder/images/600x400.png
is added as a managed file and copied to the root of public://
$file = \Drupal::service(FileRepositoryInterface::class)->writeData(\file_get_contents(__DIR__ . '/images/600x400.png') ?: '', 'public://600x400.png');
Questions:
Is the file being copied?
Where is it being copied to?
Second Step
A media entity is created referencing the managed file created in the prior step.
if (MediaType::load('image')) {
Media::create([
'bundle' => 'image',
'name' => 'Default placeholder image',
'field_media_image' => [
[
'target_id' => $file->id(),
'alt' => 'Placeholder image',
'title' => 'Placeholder image',
],
],
])->save();
}
The "Default placeholder image" entity pictured below should exist and the preview should actually provide an image .
Questions:
Does this media entity exist?
Is it previewing a valid image/path?
If everything checks out above, please share path of public:://
on the site that is failing on this.
The sdc has the preview image path configured as - src: /sites/default/files/600x400.png
which seems like it's making some assumptions regarding the stream path so I'd like to rule that out if there's nothing earlier in the chain failing.
Re #69
This is not a problem on a fresh install. The recently changed hook_install
needs to run to create the placeholder image and associated media entity, then it works fine.
This works fine on a fresh install hook_install
needs to run to create the placeholder image and associated media entity.
bnjmnm β made their first commit to this issueβs fork.
I'm not sure how to test this now that the image component is not working
The image needed is located at experience_builder/images/600x400.png but the image component, as of a few days ago, has the image set to src: /sites/default/files/600x400.png
Obviously that should be fixed, but copying experience_builder/images/600x400.png
to src: /sites/default/files/600x400.png
should make this reviewable
I feel like I had commented here before, but perhaps not.
Is there merit in exploring the use of declarative shadowdom here?
I believe @jessebaker tried it for components instead of iframes but it was problematic having to replicate the style tags inside the template elements multiple times for each instance of a component.But because we've got a fixed set of CSS here, it might be more feasible. I'd be happy to try and spike on that if folks (particularly @jessebaker who has already done some exploratory work on it) felt it was worth exploring.
Pretty much everyone front-end focused including @jessebaker is hoping for a solution where the dialog and its content remain in the primary dom, especially if any Drupal vanilla JS is expected to work with it. Also worth mentioning that the CSS scoping/isolation of the dialog contents of this issue wasn't that challenging. Most of the complexity was related to the dialog itself, not the contents.
I did a bit of exploration with the declarative shadowdom just to be sure I wasn't prematurely ruling it out. It required overriding the insert
AJAX command to use setHTMLUnsafe
to allow <template>
tags and I ran into issues similar to the ones I encountered in another project where we tried to Shadow-or-iframe a Drupal dialog where we had to add all sorts of additional communication support to ensure the dialog trigger, the dialog contents, and the dialog itself worked properly despite those three parts not being in the same DOM - the end result was something that still had problems and was even more spaghetti-ish than the MR in this issue. I don't think jQuery UI was built for long distance relationships, especially with the level of customization the Drupal dialog API has.
That said, if there's an MR that nails this I'm not too proud to let that one be the chosen approach.
bnjmnm β made their first commit to this issueβs fork.
Just pushed a solution to #66π - creating the admin CSS defaults with a custom library instead of bringing in dependencies on the dialog open request.
#61.1 and #61.2 were addressed by adding logic to recognize top-level selectors like <body>
and .js
and adding the wrapping scope after that
#61.3 was addressed by better matching the load order of Claro. These aren't CSS files with weight explicitly set, but if they appear in a different order than listed in the library definition, some unwanted styles will get precedence.
#61.4 and #61.5 were addressed by ensuring the Claro overrides and extensions were not just applied to the Dialog library, but the entire dependency tree the dialog library
I'm going to set to needs review because theres a bunch that needs to be looked at, but there's one outstanding bit I'm still looking into: On a fresh install, the dialog contents will not be rendered by the admin theme like they are supposed to. Just a cache clear will not work - but it seems like if I visit the XB UI then clear the cache, the admin-theme-rendering works from that point forward.
Left comments on the MR bits relevant to me
Fortunately I was able to reproduce the flaky test locally by slowing things down a bit (throttling or enabling Xdebug worked). The 3500128-retry-typing-until-it-works
branch has the solution, and I've re-run it several times. It being intermittent makes it hard to be 100% certain, but this fixing it on a local where it happened reliably
component-operations had two different random fail areas, one of which was recently addressed in π Implement adding component to empty layout without dnd Active . The other, which happens to be the screenshot in the issue summary, has something pretty specific going on.
It fails when looking for a section called "The Entire Node 1". In the issue summary, that section is instead namede "Node 1"
In a separate instance of this failing, the section is named "e Node 1"
This is the code where it happens - the input is receptive enough to receive the clear()
as there are no artifacts of the default section name "Two Column section", but it looks like the fails occur when some of the initial typing used to name the section fails to register.
cy.findByLabelText('Section name').clear();
cy.findByLabelText('Section name').type('The Entire Node 1');
cy.findByText('Add to Library').click({ scrollBehavior: false });
It is a pretty specific way to fail so hopefully that means a fix is reasonably easy to achieve too.
Ah rats I should have been more thorough after bringing in 0.x. #56 is what it looks like if the theme negotiator fails to instruct the dialog to render with the admin theme and just uses xb_stark because it was triggered from an xb_stark inclined route.
I consolidated the negotiators into one and this seems to be OK now
This seems to be reliably passing with the changes I made in π Implement adding component to empty layout without dnd Active . It's difficult to be 100% sure since it is intermittent but I re-ran the tests many times and it's green every time
This seems to be reliably passing with the changes I made in π Implement adding component to empty layout without dnd Active . It's difficult to be 100% sure since it is intermittent but I re-ran the tests many times and it's green every time
I expanded the scope to more easily accommodate Gin by effectively running preprocess_html
but it looks like some work on Gin's end is probably still needed here
π
Dialog styles are not loading correctly in Experience Builder
Active
, but this can be reviewed with Claro.
This brings back every skip added in π Temporarily skip intermittently failing e2e tests Active , and I've rerun the Cypress tests many times and they pass every time.
components-operations
,empty-canvas
andpublish-button
now add a component to the empty layout by clicking the component instead of dragging it. (turns out the UI already does this! Just no affordance to make it evident)global-regions
was unskipped but no changes as the drag and drop operation is never in the canvas area. Seems to be passing- The skip added in
components-slots
earlier today was removed, but only because a skip is already present on the higher leveldescribe()
that was added last month. The scope of this issue is only to remove skips added in #3500549
bnjmnm β created an issue.
bnjmnm β created an issue.
bnjmnm β created an issue.
I have a branch in
π
Media Library dialog styling
Active
(try-using-admin-theme-without-fence
) that works great with Claro and even runs hook_preprocess_html on the admin theme. Gin still doesn't look quite right and while I think the remaining tweaks need to be done on Gin's end. However, if it seems like an XB change might be the most reasonable, lmk and we'll figure it out.
Perhaps someone could try the current MR in this issue with the XB branch I've created and we can discuss what other changes might help and in which project.
I believe this is similar to my comment in π Random failure in empty-canvas.cy.js Active where there are waits being added for endpoints to address the issue, but the problem occurs slightly earlier so those endpoints are not being requested at all.
The screenshot indicates the preview endpoint isn't being requested at all, so adding a wait on it would not improve the chances of success.
The problem is likely due to the preceding drag and drop not completing successfully. There are a few areas I recommend looking at when this is the point of failure:
Possibility one - it is not sufficiently waiting on something that guarantees the target area is droppable
This test had been flaky in the past, and was addressed by adding cy.waitForElementInIframe('.xb--sortable-slot-empty-placeholder');
as the presence of that class indicates the iframe is equipped to receive drops. This part of the UI has changed a great deal recently and it's possible this isn't a reliable indicator anymore.
Possibility two - it needs an explicitly set position option for the realDnd call
This one seems less likely to be the case since the target area is fairly large but it has fixed flaky realDnd calls in the past
And it might be something entirely different but the above are things that addressed similar issues in the past.
Does that also work with Gin after #3497793: Dialog styles are not loading correctly in Experience Builder has been fixed?
I tried it out with Gin and any problems encountered were due to the the Gin accent library implementation that is being discussed as part of that issue. See comments π Dialog styles are not loading correctly in Experience Builder Active , π Dialog styles are not loading correctly in Experience Builder Active .
It is adding CSS variables that are scoped to selectors provided by preprocess_html
- a hook that doesn't run when a media library dialog is created. I suspect they can make adjustments in Gin so it works, but XB could also make it easier if we were willing to run the main controller through the admin theme's preprocess_html()
... something best scoped to another issue if we were to do it.
And with the most recent push...
I went ahead with the option "Just load the admin theme's dialog styling in the primary document. There's a risk of leaks if the dialog stylesheets are also styling non-dialog things, but all the core/Gin styles I looked are already scoped to dialog-only. "
The end result is awfully close but not yet identical to using it outside of XB.
The spots that look off are ones that can be impacted by the height calculation that takes place when the dialog opens. I'll investigate further.
Is #46 still missing the modal dialog styles from Claro?
They are not included, the approach thus far has targeted Media Library styling (i.e. the dialog contents) but not the dialog itself. Since it sounds like the dialog itself should be styled, it's probably worth clarifying in the issue summary whether this dialog styling should apply to all core dialogs triggered from the XB UI, or specifically Media Library.
This is also the case for the iframe approach in the other MR - the iframe is inside the dialog so any assets added to the iframe will not impact the styling of the dialog it is enclosed in.
Styling the dialog itself introduces a bit of additional complexity because it's common for dialog styles to use selectors at the very top of the dialog markup (such as using .ui-dialog
) - and these are the same selectors we use to wrap the dialog contents. So this introduces a scenario where we can't simply nest, but in some cases have to &
. (and at the risk of excessive reiteration, iframing would not solve this because the iframe is inside the dialog).
Two paths come to mind to get the dialog styles consistent with the admin theme
- Find a way to leak-proof the dialog styles. I think this will boost the complexity quite a bit but I might be missing something simple. I'm sure we can figure it out, though.
- Just load the admin theme's dialog styling in the primary document. There's a risk of leaks if the dialog stylesheets are also styling non-dialog things, but all the core/Gin styles I looked are already scoped to dialog-only.
With my most recent approach, Claro is looking pretty good with the non-iframe approach, and the styles are scoped so no leak. The scoping-selector can be specified, too.
There's a library override currently in place to get it working that arguably shouldn't be needed, I'll need to investigate that further.
It also has a few problems with Gin, but those seem to be on the theme side and will likely be addressed in this issue π Dialog styles are not loading correctly in Experience Builder Active
re #45
We'd still have to figure out how to solve the problem of the buttons not being loaded in the jQuery UI modals bottom panel. Wouldn't this require moving those buttons outside of the iframe which would then mean custom styling or loading admin theme styling again? We'd also have to change how the events work I guess because now the event would be triggered outside of the iframe.
This is among the reasons I'm hesitant about the Iframe. I've had to deal with this buttonpane communication in other issues and it's pretty delicate. In this case we'd need to copy the submit buttons from inside the iframe, set them to hidden, then add the copies to the out-of-iframe buttonpane, set up handlers that communicate clicks to the hidden buttons inside the iframe, then the end result of the in-iframe clicks then need to be translated back out to the regular DOM again. It's a bunch of new potential points of failure in an already complex UI and if possible I want to avoid that π€.
Re #41
The PoC I wrote seemed to work fine. There are problems with the integration e.g removing media but that is not related to the iframe solution at all.
That's different that what I encountered and perhaps mistakenly assumed the MR was incomplete.
I just checked again and rebased with the current 0.x and I'm running into the same thing
- The expected dialog behavior of moving submit buttons from the dialog contents to the dialog button pane does not occur. (Which is also evident in the screenshot provided in #22)
- The "insert selected" button does not work. When clicked, an AJAX spinner shows up momentarily but nothing beyond that occurs
For me as an outsider itβs hard to be part of the discussion since issues that are not in this issue is being raised unfortunately.
I believe anything I referenced was specific to the information provided in #17 which was linked to. To the best of my knowledge that's the extent of what I referenced, but if there's things beyond that I failed to properly contextualize it was not intentional and happy to fill in any blanks you were subjected to.
I don't think #38 looks quite like the Media Library is supposed to look like in Gin. The goal is to provide a consistent experience for the media library regardless of whether it's being used in XB or outside of XB. I'm not keen to a specific solution, but it's hard to see how we could load the base styles in XB without them leaking, at least without modifying the CSS either in JS or PHP. Do you have thoughts on how we can solve that without hard coding the styles in XB?
If it's important that ML visual consistency inside/outside of XB is the goal here, as opposed to just ensuring it's usable, then additional steps certainly need to be taken.
Keeping in mind that Site Studio stopped iframing due to issues that could not be fixed in a reasonable timeframe π Media Library dialog styling Active , I think we can provide this consistent-visual + low-leaking if we only guarantee it with Gin (and perhaps Claro). The experience would still likely be acceptable with other admin themes, but the full visual consistency would only be available with these most commonly used admin themes. Most admin themes outside of those two already have their share of weird styling as it's very difficult to account for every theme edge case.
This could be achieved either by dynamically prefxing the admin theme CSS, or maybe just updating Gin directly.
Maybe that's controversial because it's not quite as holistic as a typical Drupal implementation, but it seems preferable to an option that broke some parts of the Media Library functionality.
I've added the MR 3471978-try-using-admin-theme-without-fence
which does not iframe or otherwise fence anything. It renders the contents of the dialog (but not the dialog itself) with the admin theme. It also provides a fairly straightforward way of configuring other dialogs to be rendered by the admin theme. I'm hesitant to go with an iframe solution unless there's evidence of specific problems that can only be addressed this way, especially given that (as mentioned in #17) Site Studio had to abandon this approach due to problems they were unable to solve despite quality talent and time being thrown at it.
Anyway, Here's what it looks like with Gin (with the fix from this Gin issue: π Dialog styles are not loading correctly in Experience Builder Active )
Note this will only load the libraries that are specifically requested by the dialog contents, so it might not necessarily load base styles of the admin theme unless they are explicitly set as dependencies of the libraries requested by the dialog contents. I'm not sure this is a bad thing - with the themes I tried it seems like the UI-essential styling is reliably provided by this approach while also giving XB some agency in the overall visual experience.
So I think the main areas of potential concern with the MR might be
- Are there concerns about the media library css contaminating other aspects of the XB UI? Risk seems minimal since the previews are already iframed and Media Library styles are typically "namespaced" via distinct classnames
- Base styles aren't necessarily loaded, as it only adds libraries explicitly added in the Media Library render array. How bad do we need this? Without the base styles, contamination concerns are significantly reduced. This also gives XB more opportunity to style this while keeping the ML-essential styling present
- Were any of the concerns that motivated this issue geared towards styling the dialog itself, as opposed to its contents? This is a different challenge...
With the suggested change in #16 this is what it looks like
It looks like this with my MR from this XB issue π Media Library dialog styling Active
I'm also aware the suggested solution is pretty similar to what is in the MR. The differentiator may be due to how library alters are cached, which we discovered in π Many css files incorrectly being loaded on /xb/ pages Active make the (sorta reasonable) assumption that a single page isn't summoning multiple themes at once.
With the disclaimer that I looked into this on a standard Drupal install with Gin/XB and not Drupal CMS I was able to reproduce the issue reported.
Then, I was able to address the majority of the problems with a single line adding variables.css to the dialog library in gin.libraries.yml
dialog:
css:
theme:
dist/css/components/dialog.css: { minified: false, weight: 99 }
dist/css/theme/variables.css: { minified: false } # adding this line fixes everything
I also have a branch in π Media Library dialog styling Active named 3471978-try-using-admin-theme-without-fence that once it (or something comparable) lands it will look even better, but I think the above puts out the major fires that led to this issue being created.
The fix in π Cannot upload new image in media library Active should fix this, but even if it doesn't it should be committed before any work proceeds on this as it addresses an underlying issue that causes all sorts of flakiness.
That issue has a fix and tests already. The final thing needed is to set up Gitlab CI so a built version of the UI app is available to FunctionalJavascript tests.