πŸ‡ΊπŸ‡ΈUnited States @amanire

Account created on 22 November 2005, over 18 years ago
#

Recent comments

πŸ‡ΊπŸ‡ΈUnited States amanire

Marking as Fixed since this is in the 2.0.2 release.

πŸ‡ΊπŸ‡ΈUnited States amanire

Good to see progress here! FWIW I just deployed the patch in #48 to a production site and it resolved the deprecation warnings described in https://www.drupal.org/project/profile/issues/3324465 πŸ“Œ [PHP 8.1] Deprecated function: unserialize(): Passing null to parameter #1 ($data) of type string is deprecated in Drupal\Core\Entity\Sql\SqlContentEntityStorage->mapFromStorageRecords() Closed: duplicate . I was a little surprised at the substantial CPU performance improvement for the site, presumably because these were invalidating the varnish cache.

πŸ‡ΊπŸ‡ΈUnited States amanire

I don't understand the comment in #6. I see the ' $input.attr is not a function' error after applying the MR!34 patch. The patch in https://www.drupal.org/project/context/issues/3374119 πŸ› Search is broken when placing block RTBC resolves the issue for me.

πŸ‡ΊπŸ‡ΈUnited States amanire

MR!36 works great, thank you!

But this issue is more widespread than the description here and in https://www.drupal.org/project/context/issues/3355665 πŸ› Uncaught TypeError: $(...).once is not a function RTBC indicate. It took me a few tries to track this issue down because of the title. I'm revising it and description for clarity to help others in this situation and better expose its severity to the module maintainers.

πŸ‡ΊπŸ‡ΈUnited States amanire

Sorry that wasn't clear. My site's custom theme. I think with core, it would just result in <div></div>. In my particular case, we have an additional div in the block template which monkeyed with the spacing.

πŸ‡ΊπŸ‡ΈUnited States amanire

This is a great little module. An important UI feature that I would like is a way to filter the list of contexts. Some of the contexts are created for other purposes and are irrelevant and inappropriate to use within layout builder. I'd like to minimize any confusion for our content admins. Layout builder is confusing enough as it is! It could be an option on a per context basis but I like the idea of associating it with one or more context groups.

πŸ‡ΊπŸ‡ΈUnited States amanire

I hope this isn't forgotten. The commit is an improvement but results in a couple of nested empty divs, which were visible in my theme. I'm not sure what a better solution is though. It kind of seems like a core bug to assume the existence of the content key? As a workaround, I added this to the theme's theme_preprocess_block():

  if (isset($variables['content']['#access']) && $variables['content']['#access'] === false) {
    $variables['attributes']['class'][] = 'hidden';
  }
πŸ‡ΊπŸ‡ΈUnited States amanire

I'm looking at the code for 8.x-1.x and the patch in #8 has not been merged. This issue is not fixed and should not have been closed. It recently appeared on a project that I'm working on with version 8.x-1.5. I can't reopen this since I'm not a maintainer, so I'm debating whether I should create another issue that references this one.

πŸ‡ΊπŸ‡ΈUnited States amanire

Like @markconroy in #9, I am also seeing this on the body field of a node field widget (without paragraphs) when the "Apply filters" exposed filter is submitted via AJAX. I am also seeing this when media library is accessed via the Drupal Media button in CKEditor (in both 4 and 5). The change in cascade order of .ui-widget from /core/themes/claro/css/components/jquery.ui/theme.css overriding .button--primary from /core/themes/claro/css/components/button.css is definitely causing the display issue.

This seems related to https://www.drupal.org/project/drupal/issues/3378341 πŸ› claro.jquery.ui css assets may be added the page multiple times Fixed and may be an issue with Claro? I'm not sure whether I should rewrite this issue title and description without paragraphs and seven or create a new issue entirely.

πŸ‡ΊπŸ‡ΈUnited States amanire

This issue is a blocker for me. I would also like to know if this is ready for testing.

πŸ‡ΊπŸ‡ΈUnited States amanire

I just ran into this error again. In case others find this, I've been successfully using this drush script to remove invalid permissions without patching core:
https://www.drupal.org/node/3193348#comment-14969643 β†’

πŸ‡ΊπŸ‡ΈUnited States amanire

Thank you for linking to that write up!

πŸ‡ΊπŸ‡ΈUnited States amanire

OK this patch now includes the explicit accessCheck for D10 compatibility.

πŸ‡ΊπŸ‡ΈUnited States amanire

I've updated the patch in #80 for Drupal 10 compatibility. The corresponding feed had stopped importing and was throwing the following warning:

Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck().

πŸ‡ΊπŸ‡ΈUnited States amanire

I just tested the latest change in the MR and it works great for me at wide (1920px) and narrow (977px) viewport widths in MacOS Brave (Chromium), Firefox and Safari. Combine that with the feedback in https://www.drupal.org/project/drupal/issues/3381219#comment-15321780 πŸ› Layout Issues with Claro theme Needs work and I think we can update the status to RTBC. Nice work @simohell!

πŸ‡ΊπŸ‡ΈUnited States amanire

I'm having second thoughts about setting a max-width on the node form. One thing I noticed is that other admin forms do not have a max-width set for wider screens (for example, /block/add or /admin/structure/types/add). I agree with the typographic principle of keeping line length < ~75 characters but it also seems maybe that it’s inconsistent with the general approach of the Claro theme to use the available space. Also, I expect that the lack of a max-width would be preferred on larger screens for more complex UI elements like vertical tabs.

πŸ‡ΊπŸ‡ΈUnited States amanire

Ah, sorry, I described that incorrectly @simohell. The problem with the horizontal scrolling on narrower viewports is caused by grid-template-columns. That issue is noted and addressed in this patch and comment:
https://www.drupal.org/project/drupal/issues/3381219#comment-15238921 πŸ› Layout Issues with Claro theme Needs work

πŸ‡ΊπŸ‡ΈUnited States amanire

Ok. I updated the CSS so, that this fix does not create regression to Firefox and Safari.

If you are referring to my comment in #19 πŸ› Long string breaks the layout of Claro Needs review , I am testing in Brave 1.60.114 Chromium: 119.0.6045.124, and am still seeing the same horizontal scrollbar in other browsers. I was not describing a cross-browser issue, but a hard-coded width issue that results in scrolling on any browser viewport width between 976px and 1278px wide.

πŸ‡ΊπŸ‡ΈUnited States amanire

@simohell yes, I agree about limiting the character width on larger screens. That is also consistent with prior float-based layout so I would personally consider removing it a regression. I think this MR needs to be cleaned up a little, but I just set max-width: 76rem on node-form-layout, which results in the same main column max width as it was previously (52 rem) and restored the margin-inline: auto to center it. This eliminates that regression on larger screens.

The Title text field is set to a size of 200 in these screenshots. I'm seeing the same results in MacOS Brave (Chromium), Firefox and Safari.

Display on 1920px wide screen

Display at 977px wide viewport

πŸ‡ΊπŸ‡ΈUnited States amanire

Thank you for referencing that issue @simohell. I tested the work there locally and found that it did address the layout issue but introduced a regression in the responsive layout at viewport widths between 976 and 1278px. I posted screenshots in a comment there showing the difference πŸ› Long string breaks the layout of Claro Needs review .

πŸ‡ΊπŸ‡ΈUnited States amanire

I've been just comparing the solution in this MR against the work in #3381219 πŸ› Layout Issues with Claro theme Needs work and while it does resolve the issue with the main column expanding, it also introduces a fixed width when the browser viewport is between 976px and 1278px wide. On narrower viewports, the user must scroll to the right to view and edit the secondary column. I think we should consider this a regression, since the prior float-based layout was fully responsive and compressed the main column. I've confirmed that the scrolling appears in multiple browser engines.

I don't know, maybe I'm biased. But I prefer this approach from #3381219 πŸ› Layout Issues with Claro theme Needs work

πŸ‡ΊπŸ‡ΈUnited States amanire

Updating the issue tag. The issue is with CSS grid, not flexbox.

And updating the title to clarify that this is specific to the node form layout.

πŸ‡ΊπŸ‡ΈUnited States amanire

I was referred to this issue by way of https://www.drupal.org/project/drupal/issues/3372054#comment-15312495 πŸ› updated Claro theme renders badly for some content type edit forms Postponed: needs info . I can also confirm that the patch in #23 resolved the layout issues I described in that issue with text fields of size > 100. Looks like we need a MR for the patch. I can do that momentarily.

We should probably clarify in the issue description that this is relevant on any node add/edit form with a text field that overflows the width in Claro and not specific to sub-theming.

In addition to Chromium and Firefox, I can confirm that the patch resolved it in MacOS Safari.

πŸ‡ΊπŸ‡ΈUnited States amanire

Yes, thank you @klidifia! The patch in https://www.drupal.org/project/drupal/issues/3381219 πŸ› Layout Issues with Claro theme Needs work resolved the issue that I described above and I'm pretty sure is what the original reporter identified. I'm going to mark this as a duplicate to drive others to that issue since it has more followers and is patched, even though I think the title for this issue might be more appropriate.

πŸ‡ΊπŸ‡ΈUnited States amanire

I'm experiencing this bug as well. If tracked this down to plain text fields with a Textfield size > 100.

Noting that for text fields within collapsible form groups, this bug is perceptible for Textfield size > 87. I am pretty sure that this depends on the browser viewport size though. I am using 1352 Γ— 768px in this case.

πŸ‡ΊπŸ‡ΈUnited States amanire

I'm experiencing this bug as well. If tracked this down to plain text fields with a Textfield size > 100. I was able to clearly reproduce this on a 1920 Γ— 980px viewport size by:

  1. Creating a new content type with only a Title and Body field
  2. Going to "Manage form display" for the content type and changing the Title field Textfield widget "Textfield size" to "200"

In fact, this can be achieved directly in the browser with developer tools by simply changing the size attribute directly to "200".

It appears that the size attribute on the <input> element somehow overrides the max-width: 100% for .form-element causing the grid column width to expand accordingly. The text field itself does not expand.

I've confirmed that this occurs in MacOS Brave, Safari and Firefox, so it is not browser-specific.

In the attached screenshot, I've set the size attribute to "130" to show the impact on the layout of the secondary region on a viewport size of 1352 Γ— 768px.

I have a couple of workarounds, neither of which is satisfactory or generalizable:

  1. Add this CSS style rule, which sets all text fields to the maximum width of the content region:
    .form-element--type-text { width: 100% }
  2. Configure the Textfield size display to always be 100 or less.
πŸ‡ΊπŸ‡ΈUnited States amanire

Yes!! Thank you Rajab Natshah! The patch in #5 applied and resolved the error for me on 8.x-1.12 with Drush 12.4.0.0.

I can't confirm the 2.x branch so leaving this status as "Needs review".

πŸ‡ΊπŸ‡ΈUnited States amanire

The patch in #7 applied to 8.x-2.0 for me. Thank you!

πŸ‡ΊπŸ‡ΈUnited States amanire

Thank you for linking to that Chromatic post @gaddman! I was able to apply the patch in #2 against the stable 2.0.1 version using

        "preferred-install": {
            "drupal/insert_view": "source"

since I strongly prefer not to install a dev version on production. Hopefully a maintainer will merge this MR and release a stable Drupal 10 compatible version soon.

πŸ‡ΊπŸ‡ΈUnited States amanire

Thank you for checking @shiv_yadav. I don't know if it matters, but I'm currently running Drupal 9.5 on PHP 7.4. I updated the description accordingly.

πŸ‡ΊπŸ‡ΈUnited States amanire

Another reminder to please create a Drupal 10 compatible release. I am on the verge of removing this module from a dozen sites, which I'd prefer not to do.

πŸ‡ΊπŸ‡ΈUnited States amanire

Restating since I updated the previous comment. #2 works for me and follows Drupal documentation for hook_post_update_NAME.

πŸ‡ΊπŸ‡ΈUnited States amanire

Patch in #4 worked for me. Sure would be nice to get this into an official release.

facets 8001_add_hierarchy_process post-update Add the hierarchy processor to facets that have the hierarchy

πŸ‡ΊπŸ‡ΈUnited States amanire

I think this is a duplicate of https://www.drupal.org/project/upgrade_status/issues/3345997 πŸ› Package x is abandoned Fixed ?

πŸ‡ΊπŸ‡ΈUnited States amanire

I've just been experimenting with scroll-margin-top with this module and, unfortunately, it doesn't seem to work as expected when opening to an anchor ID inside a new page. It works fine when that page is already loaded and the anchor link is merely scrolling. I guess that this has to do with the the fact that the IDs are injected into the page with JavaScript after the DOM has already loaded. I would love to be proven wrong, but unfortunately, I think I'll have to find a way to add the IDs server-side to account for this user experience.

πŸ‡ΊπŸ‡ΈUnited States amanire

@lauriii I assume you mean in in disclaimer.component.yml? Happy to do that if you can confirm.

πŸ‡ΊπŸ‡ΈUnited States amanire

This passed testing after enabling the SDC module.

πŸ‡ΊπŸ‡ΈUnited States amanire

Aha! That's it. So I think we just need to add SDC to core/profiles/demo_umami/demo_umami.info.yml.

πŸ‡ΊπŸ‡ΈUnited States amanire

#block-umami-branding is missing? That's very odd, since I had started working on that component first but didn't think I had committed anything and the merge request diff only shows work on the disclaimer. I'll take a closer look when I can find time.

πŸ‡ΊπŸ‡ΈUnited States amanire

Yes! I agree this should at least be configurable. In the meantime, I'll use the patch.

Production build 0.69.0 2024