🇮🇳India @sandip

Account created on 20 September 2024, 10 months ago
#

Recent comments

🇮🇳India sandip

I am again moving it to RTBC as the MR is clean after the rebase. Here is the After image: After Image Link that i already posted in #26.

One minor thing is if i am not wrong the CR should be updated for version 11.3 right?

🇮🇳India sandip

As there is no response from @vasantha for one week so i am picking it up.

🇮🇳India sandip

I tried to reproduce the issue as mentioned the steps in Media Library Edit module's homepage. But the issue is not reproduceable After clicking the Edit Menu button the Modal appears and hovering on the close button of the modal no regression is occuring. I am attaching an image for better understanding.

🇮🇳India sandip

@mgifford, basically two header is rendering in the table header one is the main table header and another one is sticky header. It seems like the tab is moving twice to the header but actually the tabbing is coming for both sticky header and original header. Some how using js i tried to remove tab effect for original header but in the radio button still the tab effect is there.

I am attaching an gif to prove that the double tab wave is occuring for two table header.

🇮🇳India sandip

I have reproduced the issue and after applying the MR it fixes the issue. Attaching a gif for better understanding.

🇮🇳India sandip

Hi @nod_, I have created the MR. As @nest is removed from all stylesheets in latest 11 version but it is still in use in 10 versions. So i thought not to use @nest here as it is removed in 11 versions.

Please share your thought if it is correct or do i need to add @nest here for 10.6.x

🇮🇳India sandip

I am working on creating the MR for 10.6.x

🇮🇳India sandip

Unassigning myself from this issue as I am unable to proceed with the fix.

🇮🇳India sandip

Hi @arunsahijpal, it is supposed to test the issue in Olivero theme. By looking at your attached image it seems you are testing it in Claro theme.

I tested the issue in local and as the conflicts is resolved and the before and after images are identical to the one that I already attached in comment #8, so I’m not reattaching any images to avoid unnecessary polute.

So moving this issue to RTBC.

🇮🇳India sandip

Sorry @mgifford, I totally forget about that issue. I am unassigning myself as i am not able to fix the bug.

🇮🇳India sandip

As the width of the modal is fixed width of 300px so again the heading is wrapping into two line. Please review the changes

🇮🇳India sandip

I am working on it. I have worked on the same issue for Olivero https://www.drupal.org/project/drupal/issues/3530198 📌 The title of a dialog modal is getting truncated Active . So i would try to implement the same approach here also.

🇮🇳India sandip

Actually, I didn’t need to create a new library as the necessary one is already present, but I missed it yesterday. The truncation issue is now resolved. However, the width of the dialog modal is set to 300px, and at that width, it's not possible to keep the heading on a single line. As a result, the heading is wrapping onto two lines. Could you please review it and let me know if it needs any changes.

🇮🇳India sandip

I saw this line is introduced in this issue queue https://www.drupal.org/project/drupal/issues/3484564 📌 Define the 3 areas the Top Bar will provide Active . Here is the MR -> MR Link.

But it is not clear to me what issue it was fixing by decreasing the height from 100vh. Can you please look into it.

🇮🇳India sandip

@rkoller, Thanks for the reply. So I am raising a MR with the approach I mentioned above within tomorrow so it would be great to review then.

🇮🇳India sandip

Yes we just need to remove this below css from Gin codebase that is overriding padding-block value.

.admin-toolbar__header {
   Padding-block-start: var(--gin-spacing-xs);
}

As I mentioned in my comment #28.

I think it would be better to fix it in Gin otherwise maybe we have to use !important here.

🇮🇳India sandip

While investigating the issue, I finds out the css is coming from core/assets/vendor/jquery.ui/themes/base/dialog.css. There is no dialog.css in olivero so i think it would be better to create a library for dialog in olivero, similar to how it is handled in the Claro theme. So should i create the library in the Olivero?

The truncating issue would be fixed by removing the text-overflow: elipsis and white-space: no-wrap from h1 tag. This style is coming from core/assets/vendor/jquery.ui/themes/base/dialog.css.

🇮🇳India sandip

Fix the css selector a bit now it is ready for review on latest 5.0.x.

🇮🇳India sandip

After applying previous changes in latest 5.0.x the design is breaking. I am working on fixing it.

🇮🇳India sandip

I think may be we can remove the line block-size: calc(100vh - var(--drupal-displace-offset-top, 0px));

Before:

@media (min-width: 64rem) {
  .admin-toolbar {
    block-size: calc(100vh - var(--drupal-displace-offset-top, 0px));
    transform: none;
    inset-block-start: 0;
  }
}

After:

@media (min-width: 64rem) {
  .admin-toolbar {
    transform: none;
    inset-block-start: 0;
  }
}
🇮🇳India sandip

I go though the issue and got that this issue is occured beacuse of block-size: calc(100vh - var(--drupal-displace-offset-top, 0px)); see the codebase link Code Link in line 95. In olivero frontend theme --drupal-displace-offset-top is set to 64px that's why navigation bar height is not getting 100%. In claro Admin theme i saw that --drupal-displace-offset-top is set not defined so we can see full height of navigation bar in claro. I am attaching images for clarity. Please share your thoughts.

🇮🇳India sandip

@divya.sejekan, as i mentioned in #27 and #28, Gin is overiding padding-block-start in admin-toolbar-header class of naviation module. I also attached the image in #28 for clarity. We should make a issue there in Gin to remove the override part from Gin. Can you please go through the comments #27 and #28 and attached images once.

🇮🇳India sandip

@smustgrave, I have implemented your suggestions please review the changes. Here is the updated CR: https://www.drupal.org/node/3517675

🇮🇳India sandip

Hi @stborchert this issue is fixed in this issue https://www.drupal.org/project/gin/issues/3522015 🐛 Paragraph Active . it is still left to be merged though. Can you try this MR https://git.drupalcode.org/project/gin/-/merge_requests/611 if its works for you or not.

🇮🇳India sandip

@divya.sejekan, see if we remove the override part that is coming from Gin then it is working fine. This is why i think the issue you mentioned is Gin specific issue. Please see the attached image here.

🇮🇳India sandip

Hi @divya.sejekan thanks for the testing but it seems you are testing it in Gin theme. Now in gin it is overriding padding-block-start value that you can see in the attached image. As i changed the padding-block-start to padding-block so the padding would come for both top and bottom. So i think we can fix that in Gin. I am attaching the image for better understanding. I am again moving this to NR to hear your thought.

🇮🇳India sandip

Tested the MR in local and Code changes looks good to me so moving to RTBC.

🇮🇳India sandip

@finnsky, Please review the changes. I am attaching before and after images.

🇮🇳India sandip

Hi @everyone i observed this issue. As mentioned in #23 and #9 it is clear that we can not increase the z-index of #toolbar-bar. But some of the sections has z-index around 999 upto 99999. Thats why we are facing this issue. Here the patch at #17 i dont think it is properly solving the issue. The same issue is occuring for CK Editor also. Here is the the issue queue of CK Editor -> https://www.drupal.org/project/admin_toolbar/issues/3426402 🐛 zindex issue between admin toolbar and ckeditor 5 Active

So i think if we cant increase z-index of #toolbar-bar so can we apply the same approach to its parent div which is #toolbar-administration ?
Like we can use position: relative and z-index around 99999.

🇮🇳India sandip

Hi @yorickdv, I have reviewed the MR 622. but there is pipeline failure there because changes should be in scss and css both. Can you please make changes in the scss and compile that properly.

But i find some issue with your approach, In mobile view the buttons are not wrapping they are overflowing from the container. I am attaching Video for refernece.

So i am moving this issue to NW.

🇮🇳India sandip

I have reproduced this issue. I create a custom form and create this field to reproduce this issue.

$form['textfield'] = [
      '#type' => 'textfield',
      '#title' => $this->t('Another Text Field'),
      '#description' => $this->t('This is another text field for testing.'),
      '#description_display'=> 'before',
    ];

Default value of #description_display is after. So i set it to before to reproduce this issue.

I am attaching before and after images here you can identify description class is not attached when description_display is set before.

I am attaching core issue queue related to this issue here. https://www.drupal.org/project/drupal/issues/2700439 🐛 Description class not added to form-element.html.twig template Needs work

Moving this issue to RTBC.

🇮🇳India sandip

I think we dont need to keep both the css classes as there is no usage of .sticky-enabled is left. So i keep only .sticky-header.
I've attached before-and-after screenshot. If you inspect the "after" image, in DevTools you can see that the sticky-related CSS is correctly applied via the .sticky-header class.

🇮🇳India sandip

I have done the changes as mention in #23. Please review the changes.

🇮🇳India sandip

@nod_ sorry for the delay. I've raised the MR and attached some images for clarity. Moving the issue to NR.

🇮🇳India sandip

Hi @catch, @mherchel
Can we try something like this below. Is it the right approach for it?

diff --git a/core/themes/olivero/olivero.theme b/core/themes/olivero/olivero.theme
index b2f3bff2684..0ee7fed1c81 100644
--- a/core/themes/olivero/olivero.theme
+++ b/core/themes/olivero/olivero.theme
@@ -365,6 +365,7 @@ function olivero_preprocess_field(&$variables): void {
 
   if (in_array($variables['field_type'], $rich_field_types, TRUE)) {
     $variables['attributes']['class'][] = 'text-content';
+    $variables['#attached']['library'][] = 'olivero/olivero.table';
   }
 
   if ($variables['field_type'] == 'image' && $variables['element']['#view_mode'] == 'full' && !$variables["element"]["#is_multiple"] && $variables['field_name'] !== 'user_picture') {

New change:

/**
 * Implements hook_preprocess_HOOK().
 */
function olivero_preprocess_field(&$variables): void {
  $rich_field_types = ['text_with_summary', 'text', 'text_long'];

  if (in_array($variables['field_type'], $rich_field_types, TRUE)) {
    $variables['attributes']['class'][] = 'text-content';
    $variables['#attached']['library'][] = 'olivero/olivero.table';
  }

  if ($variables['field_type'] == 'image' && $variables['element']['#view_mode'] == 'full' && !$variables["element"]["#is_multiple"] && $variables['field_name'] !== 'user_picture') {
    $variables['attributes']['class'][] = 'wide-content';
  }
}
🇮🇳India sandip

I checked the MR and it resolved the issue and we dont need to reset css provide by .button class in .button--danger class. So moving this issue to RTBC.

🇮🇳India sandip

I have made the suggested changes please review the MR.

One thing i noticed Do we still need to attach css/classy/components/tablesort.css to the drupal.tablesort library?
I've added a tablesort.css under the components folder of the Claro theme and attached it to the same drupal.tablesort library. Just want to confirm whether the Classy version is still required.

🇮🇳India sandip

This issue is reproducible, and the MR effectively resolves it. Previously the Revision block was not rendering in the sidebar, as shown in the attached "before" image. After applying the fix, the block renders correctly, as seen in the "after" image.

Based on this, I am moving the issue to RTBC.

Before:

After:

🇮🇳India sandip

I am not able to progress more with this issue so unassigning myself.
Attaching some gif related to this issue in IS for better visibility.

🇮🇳India sandip

@wim leers, there are some unit tests are still failing. I am not getting how to move forward. Can you see the MR once.

🇮🇳India sandip

I am working on it. Before that i want to make sure are we supposed to work on 11.2.x-dev or do we need to change it to 11.x-dev. Can it be confirmed once?

🇮🇳India sandip

We dont have to remove the inheritdoc.
And as per phpcs, it is not suggested to add a sentence more than 80 words in a single line.
Phpcs will give this error - the line should not exceed 80 characters. So it will be better to divide it into 2 seperate lines.

Yes this is correct but i think those are not related to this issue. But yes we can keep it here it depends on the maintainer.
Here some unit tests are still failing if you want you can work on them.

🇮🇳India sandip

@snehal-chibde, but i recheck this on latest 11.x version of Drupal again but this issue still persists. Lets wait for other reviewers if they also able to reproduce it.

🇮🇳India sandip

@jatinGupta40, Can you please confirm is there any reason that you reverted my changes?
I fixed all your unrelated changes that was causing phpcs pipeline to fail and also made more changes that are needed for this issue queue.
Can you please kindly share your input here

🇮🇳India sandip

@JatinGupta40, you reverted all of my changes and brings back all your previous unrelated changes. Why?
Now again phpcs and phpstan pipeline is failing.

🇮🇳India sandip

I think there is still some unrelated changes are there i am working on it.

🇮🇳India sandip

I think we can move this issue to RTBC as the MR provided by @joville fix the issue correctly as i mention in #8 🐛 Dropdown does not show desired arrow on open state Active

🇮🇳India sandip

After running the failed tests the pipeline is green now.

🇮🇳India sandip

Hi @snehal-chibde you are supposed to test this issue in Gin. The image you have provided it seems claro theme.

🇮🇳India sandip

Hi @@piridium, i observed in /add page it is looking good but in /edit page drag button is not in center.

🇮🇳India sandip

Yeah sure @piridium i am reviewing this one.

🇮🇳India sandip

@wim leers, Thanks for the issue:) I noticed someone has already taken care of this issue before me. Appreciate the opportunity. I'll keep an eye out for more tasks like this 😊

🇮🇳India sandip

It seems the test failures are not related to this issue so moving to NR. Please review the changes. Attaching before and after for better clarification.

🇮🇳India sandip

I have rename all the SdcProp usage but still some tests seems failed. I am not getting why it is so. @wim leers and @ larowlan can you please take a look into it.

🇮🇳India sandip

@piridium, please click on the Get Push Access button beside the fork. So you can push your changes.

🇮🇳India sandip

Hi @kanchan bhogade, i think the failure of pipeline is not related to the file changes in the MR. Can you please check it once.

🇮🇳India sandip

Hi @joville, Reproduced this issue in my local successfully. To reproduce this issue i make olivero as Admin theme and go to the /admin/structure/block page. Also changes in the MR not seems to effect any regression. Moving this to Needs Review so maintainers can share their feedback on it.

🇮🇳India sandip

Moving this issue to RTBC as there is no regression with this MR and it resolves this issue properly.

🇮🇳India sandip

Hi @piridium, @mudasirweb
I think the best solution for this issue has landed on this issue https://www.drupal.org/project/gin/issues/3522015 🐛 Paragraph Active . Also the remove button is properly aligned with the operation button.

table.table-file-multiple-widget tbody td {
  height: auto;
  padding: var(--gin-spacing-density-m) var(--gin-spacing-m);
}

We do not need to apply this above for fixing the overflow issue. Please see this issue and share your perspective.

Here is the image after the fix: https://www.drupal.org/files/issues/2025-05-02/Screenshot%20from%202025-05-02%2011-26-20.png

🇮🇳India sandip

Hi @marco.pagliarulo, thanks for the MR it resolves the issue properly and it also fix another issue for overflowing paragraph fields attaching SS for refernce.

🇮🇳India sandip

Hi @nupur badola, Please make sure to set Number of levels to display > 1 in configuration block of Main Navigation menu.

🇮🇳India sandip

Patch at #2 is working fine. I create a MR for that patch.

🇮🇳India sandip

Also this line is not making any required changes in MR548. It is not needed.

.form-managed-file.has-value.is-multiple {
        display: flex;
        gap: 1rem;
        .form-managed-file__meta {
          align-items: center;
        }
      }
🇮🇳India sandip

Removed out of scope changes. Please review the MR.

Production build 0.71.5 2024