🇮🇳India @yash.rode

Tests are passing now. Ready for review.

#3023322-134: Contextual Links Style Update → We cannot use floating UI lib, that is a functionality change and this is specifically for styling.

yash.rode → made their first commit to this issue’s fork.

Hi, I also tried providing the {"allowOther": true} option to no-sizzle. But couldn't find any way to do so. The other way I can think of to do this is -- adding inline comment to Eslint disable for the lines which uses :visible and get rid of those once we figure out a solution for :visible?

yash.rode → made their first commit to this issue’s fork.

It is mainly for the accessibility users, so it makes more sense for it to be just above the table instead of going to theme settings page and changing it there.

#3445425-18: \Drupal\Core\Template\Attribute should implement Countable interface → is not true, the test is failing without the fix. Attached is a test only patch.

yash.rode → made their first commit to this issue’s fork.

Hi I tried the steps above but the issue only occurs when we try to change the URL and not when we visit the URL for the first time?
If that is the expected behaviour how can we write a test for that scenario?

How can we apply javascript deprecation → on top of this session storage key?

Hi, I was trying to reproduce this issue in Drupal Core, where I can click on any button which takes me to the link with a fragment?
I am trying to write the test, but not sure what should be the Drupal page where I can get this error which I can convert to test.

yash.rode → made their first commit to this issue’s fork.

As patch testing will be stopped form 1st july → .

yash.rode → made their first commit to this issue’s fork.

It is not working for stark theme, I will have to debug that more to figure that out.

This is ready for another round of review.

yash.rode → made their first commit to this issue’s fork.

Hi @bnjmnm, I tried the steps from #3270083-18: Some theme hooks are not invoking (depends on templates order provided by filesystem) →

In other words, this can be tested by adding a custom module or theme that provides templates with a directory structure where the template that should appear first is in a subdirectory of the directory with the templates it should appear after.

But it was loading

Main Directory Template

instead of subdirectory one.

if return array_merge(array_merge(...$files_in_sub_dirs), $files_in_this_directory); is reversed i.e return array_merge(, $files_in_this_directory, array_merge(...$files_in_sub_dirs)); , then it will load the sub directory one.

I am working on adding the test.

Re-created the tableheader.js for making the sticky optional. Added the sticky header checkbox before the sticky header.
need to be cleaned and use local storage to handle the page refresh.

yash.rode → made their first commit to this issue’s fork.

I was able to reproduce the issue with Paragraphs, but as we cannot write test for paragraph I was trying to reproduce the same thing for Custom block but unfortunately I am unable to do so. can someone take a look at this?

This is the working patch for Claro which encorporates suggestions from #3163765-7: Add option to un-sticky table headers to benefit assistive tech users → ,
but unfortunately 📌 Make drupal.tableheader only use CSS for sticky table headers Needs review just got in, which makes all this of no use.

Hi, the current version of the MR works for all other themes, except Claro as discussed in #3163765-19: Add option to un-sticky table headers to benefit assistive tech users → . I have the version of tableheader.js which is working for Claro also, but there are few things that needs to sanitised in that, will get to that tomorrow. Posting that as a patch for now.

I tried keeping/removing the `position-sticky` class from the table, which makes it sticky/non-sticky respectively. I will try to implement that solution.

Hi, I was trying to make the patch from #3163765-2: Add option to un-sticky table headers to benefit assistive tech users → compatible with 11.x, but while trying to apply tableheader.js's changes, I noticed that TableHeader is not getting initialised. Can someone tell me how to approach this? For now I have created a new instance of TableHeader which doesn't seem right

yash.rode → made their first commit to this issue’s fork.

yash.rode → made their first commit to this issue’s fork.

Hi, I was trying to reproduce this problem, but I was not able to do so.
I created and installed a Custom module with

/**
 * Implements hook_theme_suggestions_alter().
 */
function my_mod_theme_suggestions_alter(array &$suggestions, array $variables, $hook) {
    $suggestions[] = $hook . '__minimal';
}

and tested if the form-element-label.html.twig is replaced with the new form-element-label--minimal.html.twig.
The new template is overriding the original template. below is the snapshot showing the same

As it can be seem from the above image, I have added

to form-element-label--minimal.html.twig. Also it is loading form-element-label--minimal.html.twig instead of form-element-labe.html.twig.

TLDR either the issue does not exist anymore as the issue is quite old or I am missing something in the steps to reproduce.

#3371937-18: Theme declaration and templates mismatch for feed_icon → attached before-after screenshots.

Below are the before after screen shot which shoes that the class some-class getting added once the MR is applied.
Before:

After:

We are not able to read using fs so we have used fetch to do so. In the Stand alone react app we are able to read a file, within the react app. what should be the next step on this?

yash.rode → made their first commit to this issue’s fork.

dev, build:react-dev and build:react all three scripts are running with vite now.

If we refresh the page with current MR, we are getting Using the "css" tag in runtime is not supported. Make sure you have set up the Babel plugin correctly. this error, in the previous version where we were using esbuild

linaria({
      esbuildOptions: {
        jsx: 'automatic',
    },
})

this code in create-react.mjsfixed the same problem, what is the equivalent of this in vite.config.ts?

I am following Migrate CRA to Vite, it is not completely working yet, but can someone verify if I am on the right track.

yash.rode → made their first commit to this issue’s fork.

yash.rode → made their first commit to this issue’s fork.

Hi, I tried the current state of MR on my local, it is working for the test modal page but it is not working for the layout builder Add section page.

Update queries to ignore elements inside drupal-fragment and other custom elements.

Tried this, still Hello world button does not work.

While looking at prepareDialogButtons($dialog)
Looking at the function which is calling prepareDialogButtons is same as the core's but, Drupal.AjaxCommands.prototype.openDialog get's only one button when we try to call prepareDialogButtons($dialog). We should get all the three buttons just like non-jsx.

but in non-jsx, we are getting all the three button in

if (
      !response.dialogOptions.buttons &&
      response.dialogOptions.drupalAutoButtons
    )

and in our case there we get no buttons in this if and getting only one(preview) button in the next call to const buttons = Drupal.behaviors.dialog.prepareDialogButtons($dialog);. So, Is that a expected behaviour or a there is something wrong?

We also added Drupal.attachBehaviors for the nested components, but it does not make any difference as of now.

Hi, I am trying to implement

When the JSX theme engine is in use, delay the initial dom-ready call to Drupal.attachBehaviors and have it run once the initial JSX render occurs. Perhaps an event ca...

I need help with that,
we want to dispatch an event at the end of index-react.js::processJSX with some timeout, so that all the React elements gets rendered and then we want to call the Drupal.attachBehaviors in drupal.init.js once the event is triggered from the index-react.js::processJSX? Is this what you are trying to say in previous comment?

Hi @bnjmnm, I tried debugging this, I think there is something wrong with Drupal.behaviors.AJAX's attach method, first thing we need to change the timeout to 50000000000ms(lesser than this might also work but 50 doesn't work) so that the element get's available and we can add `drupal-ajax` to it.
I was able to add the `drupal-ajax` but still the functionality is still not working, I further went on debugging ajax.modified.js's Drupal.ajax but it is working same as non-jsx. So I am not sure what's missing in there. It would be helpful, If you could take a look and let me know what could be the next step on this.

Removed the

if (childNode.hasAttribute('data-drupal-scriptified')) {
          break;
        }

blcok, which solves this issue, but fails tests. As well as when tried with the Ajax test module, some of the functionality does not work.
example: when we open the Link 5 (form) on ajax-test/dialog page-- and then click on the Hello world button we expect a Hello world status message, but it does not work.

#3432283: Layout Builder "Add Section" does not have submit button → will fix this issue also, as both of them were having a similar problem. Once that issue is merged this will also start to work.

yash.rode → changed the visibility of the branch 3432283-layout-builder-add to active.

yash.rode → changed the visibility of the branch 3432283-layout-builder-add to hidden.

yash.rode → made their first commit to this issue’s fork.

yash.rode → made their first commit to this issue’s fork.

I see that this is a very large change 12 files + 728 − 69, larger than the recommended change size. That makes this challenging. Is there any way to reduce the size? I made two comments on the MR and both require work.

not sure about what can be done.

There are also other unresolved threads in the MR some of which were not minor changes.

All the threads are addressed, but I cannot close them, please close them who created the MR.

Other than this, I have addressed all the feedbacks.

Addressed #85's first point and

Another thing that I noticed is that if we click on the edit machine name, the edit machine name form element appears after the description, but ideally it should be next to the Name form element.

this is the existing form, so if we want to do this we should do that in a separate issue.

All the feedback has been addressed.

Addressed feedback and responded to #79

Thanks for the review @benjifisher,

Also, I am not satisfied with the response to Comments #75, #76. If I read them correctly, they are saying that the new test for this issue does not fail as expected when run without the non-test changes here. If that is the case, then it is not a useful test.

this is not right, the test for this issue won't pass without the not-test code. But the test will pass if we skip https://git.drupalcode.org/project/drupal/-/merge_requests/4839/diffs?co.... Reason for that is explained in #77

Updated CR and addressed feedback.

yash.rode → made their first commit to this issue’s fork.

As everything is working as expected.

I think https://git.drupalcode.org/project/drupal/-/merge_requests/4839/diffs?co... is not must have for all the setup but a good to have as it was not showing the display modes in the local task bar for some set-ups. That's the reason for #75.

Hi @borisson_ can you please have a look#204? I have already added test coverage for that.

@alexpott, did you get a chance to look at the approach you suggested detecting config override https://git.drupalcode.org/project/drupal/-/merge_requests/3286#note_243097?

Hi, after spending some time on this, I think tests on #19 are passing because of https://www.drupal.org/project/drupal/issues/296693#comment-15328018 🐛 Restrict access to empty top level administration pages Fixed , and I think we should have not included that in the test only patch.

@smustgrave would be a good fit to review this one, as he was able to reproduce it on local.

Small suggestion regarding the current approach.

The test coverage looks adequate and assertUserRoutesAccess() is giving quite easy to understand error messages.

Addressed #201, if that looks good https://www.drupal.org/node/3375145 → can be deleted.

#207 that issue is already in, so no need to postpone.

Added test coverage and fixed test.

With the fix ^ we can now debug the code in \Drupal\system\Access\SystemAdminMenuBlockAccessCheck. Can someone tell the next steps as it is not clear from the current IS or any comment. Thanks in advance.

yash.rode → made their first commit to this issue’s fork.

Is there any example of ConfigFormBase where sequence is used
or is the below statement correct?

I am not sure, but when I did $config['update.settings']['notification']['emails'] = ['a@abc.com', 'admin@example.com']; in settings.php, I am getting what is shown below:

If it is correct I will write test for this, otherwise can you please suggest any of the existing form which I can write test for.

I tried accessing /admin/structure/block/manage/olivero_powered and adding setting related to that to settings.php, but not getting the status message(Not totally sure about what I have added to settings.php-- $config['block.block.olivero_powered']['visibility']['request_path']['pages'][] = ['/randompage'];). Tried putting a breakpoint in, core/lib/Drupal/Core/Form/ConfigFormBase.php it is not going to the breakpoint, what can be the reason?

Addressed @narendraR's feedback.

yash.rode → made their first commit to this issue’s fork.

addressed #14

All the threads have been addressed. Except the local task issue, which I am not able to reproduce.

yash.rode → created an issue.

All the threads are addressed.

I have addressed the feedback, except https://git.drupalcode.org/project/drupal/-/merge_requests/4841#note_234897. Does anyone have any alternative way of doing this.

I have tested this, it is working as expected and test coverage looks good to me.
There is one small documentation nit, after which this is good to go.

The change in tests is regarding the lines where we tried checking if the focus is on the form/content, but now we need that focus to be on the first tabbable element present on that modal.

yash.rode → made their first commit to this issue’s fork.

Tested this manually, It is working as expected and the test coverage is also thorough.