Belgium 🇧🇪
Account created on 7 January 2016, almost 10 years ago
#

Merge Requests

More

Recent comments

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I still believe this is done by whitelisting URL's at Wiris side. I can not find a single reference in their documentation on how to set an API key or something similar. It just mentions:

Requirements
- CKEditor 4.0 or higher installed.
- A valid license to install the integration in a production environment; otherwise, you can use the downloaded file just for demo purposes.

Since you seem to have a valid license @pierreabreup. What exactly is the key you have? Is it an API key, or simple a document stating you have a license?

Reference: https://docs.wiris.com/en_US/mathtype-for-ckeditor

I asked ChatGPT if he knows how to set a license for Ckeditor Wiris, but it's just making things up 😅

🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I was not able to get the element to delete itself during the recording. However as you can see, the editor freaks out when duplicating an element.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪

Seems to be an issue in the JS function updateIframeSize which keeps triggering the function onBodyResize

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Yeah, the class logic doesn't seem to position the button as fixed anymore on 2.x

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Does this impact the front-end editing experience, or rather bring frontend styles to the backend edit form? Screenshots might be helpful as well.

Basically what this does is apply the front-end CSS to your back-end editing form. It provides a config form where you can specify the specific library (as in libraries.yml) from your theme to use. There is also an option to load additional twig templates, although I haven't found the need to use this. It doesn't change anything to the editing UI, nor the front-end.

The only minor annoyance with this is that if your CSS is too broad (e.g. you themed on button, or general form field styling) it gets applied to the back-end as well.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

@sharique There is a new MR for 3.0.x

I don't think it makes sense to backport this to the 2.0.x branch as there are no fixes added to that branch anymore.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Kay.. judging from the test changing to RenderElementBase might not be the way... will look again tomorrow :)

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I created a 3.0.x branch and fixed the deprecation. The failing tests are not related to the changes here as the baseline for 3.0.x is also failing.

I accidentally borked the merge request for 2.0.x by merging 3.0.x onto that branch, I reverted that commit but GitLab is still showing more changes -.-.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

To fix #31 we need to solve this deprecation for D11 https://www.drupal.org/node/3404140 but that means bumping the min version requirement to D10.2 which I think is fine this day of age.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I tested the merge request on 3.0.0-alpha3 (on D10.4.5) and it works (and applies) fine. There are some CSS issues but that's due to our custom theme and components integration which are depending too much on the global CSS styling. I managed to get the layout editor in the back-end to look like a 1-1 copy of what the front-end is showing! Very nice :-). Now just for us to ensure the front-end styling does't alter the back-end buttons/tabs/forms etc.

Can the author of the MR set the target branch to 3.0.x?

🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪

Quickly bumped the requirements so I have a patch to test

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

This is actually already merged. Maintainer should assign contribution record and set the issue to fixed.

I would also recommend removing the statement on the module page that you need the patch.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

RE #31, if you made changes, please also apply them to the merge request. The patch workflow is deprecated. Also without interdiff, it's very hard to know what you changed.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Looks a lot better and readable. The test only run fails as expected.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Also wondering how big of a table we're talking as I also tested this on a few projects with substantial amounts of data.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

That might be one way of doing it, but the site in question already has like 15 text formats. So not sure how smart it is to duplicate some of them to just check that one box :P

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪

Updated logic, cleaned up the variable names to be in line with best practices, added a simple test which checks the image size.

🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪
🇧🇪Belgium BramDriesen Belgium 🇧🇪

I also bumped into this problem. The problem is that the data-pswp-width and data-pswp-height are being calculated from the image in the ckeditor, but not the source image.

This needs an issue fork,

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Turns out this had to do with using "align right/left" on images which adds a class as well. Updated logic.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

A form for whom? It's the maintainer's responsibility to assign credits. Not a task for site moderators for example.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Sounds good, thanks @donquixote

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Maybe I shouldn't have said "widget" ;-) be we do have tests for TimeRanges etc.

https://git.drupalcode.org/project/time_field/-/blob/2.x/tests/src/Kerne...

I guess it should be an issue there as well?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Also spotted your comment on the parent. That should indeed not happen!

If we can add a test case as described in the proposed resolution this would be perfect. Should be easy to write a similar test for all widgets.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I'll need to test this on the 2.x branch, but can't seem to get a working env on GitPod or Simplytest with the 2.x branch 🤔

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Flagging as needs work as the old UI no longer is available. The new GitLab process needs to be described

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I think this needs to be re-issue-forked. I doubt it's possible to target a different project.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

It still needs to be tested and checked if all deprecations are fixed

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Please update the MR if there are relevant changes.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Thank you so much @miroslavstankov

Happy to merge this once the PHPCS and PHPSTAN errors are fixed :-). Assigning credits.

In case you don't know, you can see the reports in the GitLab runner results, and some are visible on the merge request diff.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I don't understand this sentence. What do you mean by "went through the update hook"?

What I ment to say is that wherever you developed this, you probably got the fields through the update hook, and not the install process that only looks at the schema file.

I'm not convinced that this will help, since Kernel tests do not use the local environment. They use a separate, empty database with a clean Drupal installation to execute.

That is exactly what I'm trying to say :) there might be a mistmatch between the schema and update hook. Hence why it might work locally for you, but not in the test runner.

I seem to have lost push access to my own issue fork

If you're going through the GitLab UI, make sure you're logged in (check user icon top left).

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Re #4: Locally you probably went through the update hook to get the fields. The test runner does this with what is in the schema.yml file. You can also try to uninstall the module and re-enable it, or try it on a fresh install. Check the status page for schema errors.

Is this correct?

field.field.time_range:
  type: field.field
  label: 'Time range field'
  mapping:
    settings:
      type: mapping
      mapping:
        require_start:
          type: boolean
          label: 'Require a start time'
        require_end:
          type: boolean
          label: 'Require an end time'

Why field.field? Should that not just be type mapping with everything below it? (just thinking out loud, didn't really look at the rest of the MR)

🇧🇪Belgium BramDriesen Belgium 🇧🇪

+1 But this is key!

The commit author address is a conscious choice by developers that should be respected by default.

As described on your git instructions page: https://www.drupal.org/user/UID/git . One has the choice to use an anonymized address.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Really nice! I haven't tested yet, but I presume that the new git commit message is ready to use?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I believe there still is the unhandled comment from @benroy on the backport MR which also applies to the 11.x MR.

Then there is the open question in #72 and open comments on the 11.x branch. Will leave it at NR for now so it hopefully get's some attention from the correct people as well to help decide on this.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

@andileco What's the status of your work? 😊 can this be reviewed?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I came here because of a slack discussion. If a review can help land an issue I will do so (a bit in the light of the boy scout rule). I actively try to avoid working on novice issues and even create novice issues for the modules I maintain for others to pick up 🙂. But reviewing a core issue in my perspective might fall outside of a novice task. Especially one like this where one might think you can just delete the two empty test cases.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Looks good to me. Also referencing the issue where those test cases where initially added and why they exist.

core/modules/system/tests/src/Functional/GenericTest.php is indeed the one to keep.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Just to highlight why this was changed back to active, this should not be a new submodule in core.

You might want to read this before working on Core issues: https://www.drupal.org/community/contributor-guide/reference-information...

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Updated and also on the event page.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

There are no merge conflicts, the GitLab diff is for 11.x (dev). Are you on the dev release of D11?

Also, you should use .diff and not .patch.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

This seems major as it's fundamental for the module.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

The MR is a blind re-roll without reading through the issue what needs to be actually done.

I'm also still not sure if we need this, maybe it has already been fixed? I guess this can be manually tested (without the patch) to see if there is a conflict with the conflict module.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Finally was able to test this manually, this works like it's expected to! Thanks again @tame4tex

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I think this needs a lot of clarification. It sounds more like a paragraphs issue. On the other hand I have no reasons to believe paragraph fields are untranslatable.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

@_pratik_ why update an outdated patch when there is an issue fork with more changes? Why upload a patch when the patch workflow is deprecated?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

@astonvictor Please link the issue where the deprecation was fixed.

📌 Automated Drupal 10 compatibility fixes Active only changed the info.yml file

🇧🇪Belgium BramDriesen Belgium 🇧🇪

More strange behaviour which might be automated or AI assisted in some way.

RTBC'ing an issue which is merged and fixed for over a year: https://www.drupal.org/project/select2/issues/3271205#comment-16191852 🐛 Removing selected option sometimes needs multiple clicks RTBC

Reviewing old/outdated patches when there are newer versions/MR's available:
- Yes, same issue: https://www.drupal.org/project/select2/issues/3271205#comment-16191852 🐛 Removing selected option sometimes needs multiple clicks RTBC
- https://www.drupal.org/project/drupal/issues/3301239#comment-16190176 🐛 PoStreamReader::readLine() throws an error on module install Needs work

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Re #54, why are you testing/reviewing a very old patch (#7) when there are rebased patches and a merge request? This feels like lost effort that could have been spent better.

Also "Works for me" is not a Drupal.org status. We have "RTBC (Reviewed and Tested By the Community). But since you're reviewing old patches, it's not really ok to make that change here after your review.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

RE #4, what did you test? There is no patch #2, the patch you are referencing (3323974-media-link-blank-target-11.x.patch) does not exist, nor here, nor in issue #3323974 which I guess is not even closely related to this one.

Did you use AI to generate your comment? If yes, be aware this is covered under the credit abuse policy. https://www.drupal.org/drupalorg/docs/marketplace/abuse-of-the-contribut...

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Re #31, what did you actually test? You're referencing the patch from #2, but also the MR  !11081 which contains more changes as the patch...

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I guess some of this can be broken down into separate issues?

🇧🇪Belgium BramDriesen Belgium 🇧🇪

Usability team would indeed be better. Thanks @quietone!

🇧🇪Belgium BramDriesen Belgium 🇧🇪

I think we're going to keep go in circles until a core maintainer helps with suggesting the correct string/wording changes for this.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

bramdriesen created an issue.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

@rahaf albawab patch workflows are deprecated. Please update existing issue forks... You provided no info or interdiff so it's unclear what you fixed, if anything. Hiding patch for that reason.

🇧🇪Belgium BramDriesen Belgium 🇧🇪

+1 For #9, I think this should be made very clear.

The discussion for adding a label for "AI generated" modules is separate discussion happening elsewhere.

Production build 0.71.5 2024