Account created on 24 February 2012, over 13 years ago
#

Merge Requests

More

Recent comments

🇺🇸United States jcandan

This makes sense. The ordered list number 4 needs to be updated to 3.

🇺🇸United States jcandan

@plopesc, thanks for your work on this.

Your test correctly failed prior to the fix, and passes after the fix. I've refactored a bit, removing the AJAX adjustments and adjusted the conditional logic for persisting the country selection. Feel free to add an issue if you'd like to describe steps to reproduce the AJAX issue your team is experiencing.

MR !14 is ready for review. Will leave this open for a couple days to receive feedback, but this looks good to me.

🇺🇸United States jcandan

I'd imagine that, until this is added to Radix, in order to adopt Storybook , we'd just have to create our own *.stories.twig files in our sub-theme, and for those, a *.compentent.yml with the replaces key pointing to the target component in Radix. Anything else I may be missing?

🇺🇸United States jcandan

Added Bump supported intl-tel-input version Active . Seems appropriate to get the front-end package dependency a good solid update before a major release.

When 2.0.0-rc2 gets released, it will introduce breaking changes (changes that require consuming applications to make code modifications to continue working correctly with the new version). While release candidates may be considered close to stable, this update is long overdue, and introducing breaking changes on a release candidate isn't strictly forbidden. We'll be sure to communicate this in the release documentation for RC2.

🇺🇸United States jcandan

@plopesc pointed out that this feature may already be addressed by Telephone Validation . I wonder if @omkar-pd might be willing to share reprucible steps if this becomes an issue with the modules not playing nicely.

🇺🇸United States jcandan

Removed 🐛 Fix validation not being applied Active as a release blocker. Willing to consider putting it back in. Awaiting community feedback.

🇺🇸United States jcandan

I realized that this should be marked as a feature request, not a bug. To fail validation and prevent save might even be considered a breaking change. Though, if the community wants this, we could add it to a release candidate before making it to 2.0.0. Perhaps even make it configurable.

Moving this out of 📌 Release 2.0.0 Needs review , unless someone objects.

🇺🇸United States jcandan

In your video, you type the exit code and country code into the field, rather than select the flag. I was able to reproduce this bug by selecting the flag.

🇺🇸United States jcandan

I was able to reproduce this issue. Digging.

🇺🇸United States jcandan

Need to verify this indeed has been fixed.

I created this ticket based on #3302638-3: Telephone International Widget 2.0.0 stable release plan . I assumed the front-end validation from 🐛 Doesn't work with multiple phone fields Needs work was sufficient, but I realize now that this is referring to back-end validation.

🇺🇸United States jcandan

Absolutely! Thanks for calling that out, @johnpicozzi. I'll take a moment to look through the other tickets which may have influenced the total sum of changes captured here. Thanks everyone for your patches, MRs, and review.

🇺🇸United States jcandan

As part of the work done in 🐛 Doesn't work with multiple phone fields Needs work , we load the utils.js as part of the *.libraries.yml.

🇺🇸United States jcandan

The composer.libraries.json has been removed as part of 🐛 Doesn't work with multiple phone fields Needs work . See that ticket for details.

🇺🇸United States jcandan

Got a test added and refactored the work from the above patches and MR !8. Good job and thanks , everyone.

Please, someone review MR !9.

🇺🇸United States jcandan

Never mind. That intlTelInputWithUtils.js doesn't even come with the package. Must be outdated docs.

🇺🇸United States jcandan

Option 1: intlTelInputWithUtils
If you're not concerned about filesize (e.g. you're lazy loading this script), the easiest thing to do is to just use the full bundle (/build/js/intlTelInputWithUtils.js), which comes with the utils script included. This script can be used exactly like the main intlTelInput.js - so it can either be loaded directly onto the page (which defines window.intlTelInput like usual), or it can be imported like so: import intlTelInput from "intl-tel-input/intlTelInputWithUtils".
-- https://github.com/jackocnr/intl-tel-input?tab=readme-ov-file#loading-th...

Given this is loaded when the widget is used, let's adopt the above implementation in lieu of the changes made from #11.

🇺🇸United States jcandan

Merged 📌 Remove the composer libraries workaround Active . This ticket can be updated to reflect any desired changes.

🇺🇸United States jcandan

Apologies for the crazy number of commits attempting just to get a working test. I had a hell of a time because I didn't think to ensure the test user was logged in to view the node/add page to test the field widget. oi vey.

But, the really cool thing is, I got a GitLab CI customization and browser tests (inspired by DropzoneJS) implemented!

This CI composer job and testing nicely covers the exact instructions now provided in the readme and ensures the widget gets the intl-tel-input flag select in place.

🇺🇸United States jcandan

Would appreciate your insight on the approach done in 📌 Remove the composer libraries workaround Active .

Because Drupal prefers third-party libraries to be accessible from docroot (web server), the convention is for packages of type drupal-library to be installed to the /libraries/ folder. This is also the place where Drupal is able to allow and manage multiple versions of packages, as dependency management for a modular project via the vendor/ directory could get unwieldy.

So, the approach in the above-mentioned ticket seems the best option, and a good replacement for the composer.libraries.json and asset-packagist options of old.

🇺🇸United States jcandan

With this release, it will include README changes that will need to be published to the project page.

See 📌 Remove the composer libraries workaround Active .

🇺🇸United States jcandan

Nice. Good to see you even made changes based on eslint in the CI pipeline.

Leave MR !11 as-is now. I am working on 🐛 Doesn't work with multiple phone fields Needs work where there will be changes to the CI and test suite we need to capture before this can have tests added.

🇺🇸United States jcandan

This is a big undertaking. While I do believe it is ideal to tackle issues distinctly, the frequency of updates to this module has made it such that a big change affecting multiple bugs is inevitable. I will review this further. I plan to add tests as described in #10. Thanks, everyone.

🇺🇸United States jcandan

I think I see what happened. It seems Ahmad's #17 patch mistakenly missed somethings, #18 is a re-roll of #12 with his desired change to INTERNATIONAL format rather than E164.

🇺🇸United States jcandan

Needs the patch rolled into an MR. Needs a regression test.

🇺🇸United States jcandan

If unable to reproduce, we can mark this closed as works as designed. Will keep this open for a couple weeks for feedback.

🇺🇸United States jcandan

Already Drupal 10 compatible. Rector appropriately fixes the public property, and submitted a patch for it to be protected. This same fix will come along with the Drupal 11 automated ticket.

🇺🇸United States jcandan

I pushed a new branch to the fork: 3349389-image-block-and-media-linkable.

There I attempt to introduce imageBlock and media as linkable models. I got about as far as the functionality introduced by !28--attributes stick, but not the target="_blank".

However, I think I now understand the reasoning behind branch 3349389-link-attributes-dont and suggest work continues there. I've learned that the htmlLinkAttributes overrides skip attempting to target specific models (e.g. imageBlock, media).

The core trick in the patch is that it doesn’t try to manually wrap images and media in tags itself—instead it piggy-backs on Drupal’s existing “HTML Support” converter that already knows how to take a htmlLinkAttributes-model attribute on an element (like a block image or media embed) and turn it into a real wrapper, and vice versa.

I'm just still not sure why the target attribute isn't being handled given this explanation.

🇺🇸United States jcandan

Rebased Merge request !28 to capture recent slew of updates to 2.2.x.

Though this captures the work in progress so far, the issue has not been resolved. Still needs work. Patch #31 likely needs a re-roll to apply correctly, but will wait until we have created a working solution.

🇺🇸United States jcandan

No merge required. It turns out that, although the 2.2.x to 2.x comparison shows the logo change as a difference, the logo change is indeed part of 2.2.x.

The 2.2.x is the most up-to-date branch.

We can safely rebase 2.x from 2.2.x, then remove 2.2.x.

Maintainer may find the following helpful.

git fetch --all
git checkout --track origin/2.x
# skipped previously applied commit 8fb5e79, reapply-cherry-picks to include skipped commits.
git rebase --reapply-cherry-picks 2.2.x
# see that the branches now match at HEAD.
git log
# delete remote branch.
git push origin :2.2.x
# delete local branch.
git branch -D 2.2.x
🇺🇸United States jcandan

We no longer need to wait to decide if the other ticket takes priority and fixes this issue. Given the recent slew of changes merged to 2.2.x, and possibly due specifically to Use built-in CollapsibleView instead of DetailsView Active , this issue seems to have been resolved.

Marking fixed. Feel free to reopen if you're able to reproduce the exceptions described above. Note: this ticket will automatically be closed (fixed) if no status change is made in 2 weeks.

🇺🇸United States jcandan

Given the recent slew of changes merged to 2.2.x, and possibly due specifically to [#16061370], this issue seems to have been resolved.

Marking fixed. Feel free to reopen if you're able to reproduce the exceptions described above. Note: this ticket will automatically be closed (fixed) if no status change is made in 2 weeks.

🇺🇸United States jcandan

You may need to run Yarn's yarn dedupe command to potentially consolidate the package to a single version if the versions are compatible.

🇺🇸United States jcandan

This introduced duplicate, higher version than core in yarn.lock file:

"@ckeditor/ckeditor5-core@41.3.1":
  version "41.3.1"
  resolved "https://registry.yarnpkg.com/@ckeditor/ckeditor5-core/-/ckeditor5-core-41.3.1.tgz#b4bf65cc1a291d78d9e47269ae159aec1451d99b"
  integrity sha512-h+PgPtCpS2vjO3HbKMYtddRPW+B3AJx9qpixmHJnUZMiFCmRjUZjXATjpi3j+kSQISs4L2Yghq+lsAQxyGHb+A==
  dependencies:
    "@ckeditor/ckeditor5-engine" "41.3.1"
    "@ckeditor/ckeditor5-utils" "41.3.1"
    lodash-es "4.17.21"

"@ckeditor/ckeditor5-core@41.4.2":
  version "41.4.2"
  resolved "https://registry.yarnpkg.com/@ckeditor/ckeditor5-core/-/ckeditor5-core-41.4.2.tgz#0779a727b0238368fc09ee4ec855536ea1ab24db"
  integrity sha512-kCIJjviiMNIMBMx7XFXFp1IeTELQKv7xyPJiVFDyUftIfthf9uWty72ipZ3BBNBGBkaoTiSzDZ507EsX6czuIQ==
  dependencies:
    "@ckeditor/ckeditor5-engine" "41.4.2"
    "@ckeditor/ckeditor5-utils" "41.4.2"
    lodash-es "4.17.21"

Perhaps, to ensure we match core, we should remove the open constraint (~) on ckeditor5.

🇺🇸United States jcandan

Use built-in CollapsibleView instead of DetailsView Active introduced a bug to 2.2.x: the tabindex problem experienced in !14, described in #28--keyboard navigation skips advanced fields.

Do we want a new issue for this, or will we tackle that here?

🇺🇸United States jcandan

Can we add the following bash examples to help with Yarn linking all packages?

<code>
cd path/to/ckeditor5/packages
for dir in ./*; do (cd "$dir" && yarn link); done
cd path/to/core
for dir in /path/to/ckeditor5/packages/*/;  do (dirname=$(basename "$dir") && yarn link "@ckeditor/$dirname"); done
</code>

🇺🇸United States jcandan

I am attempting to debug this via a source build. Requires a bump to CKEditor5 v41.3.1 due to errors mentioned in Upgrade CKEditor to match Drupal core at 41.3.1 Active .

🇺🇸United States jcandan

Running through CKEditor 5 development docs, when I am on v35.1.0, the yarn install command fails with:

error @es-joy/jsdoccomment@0.36.1: The engine "node" is incompatible with this module. Expected version "^14 || ^16 || ^17 || ^18 || ^19". Got "22.14.0"

When I am on 41.3.1, this error does not occur.

🇺🇸United States jcandan

If this really is a necessary component of the CKEditor Plugin plugin module development, is it possible to provide the Drupal GitLab CI with an NPM build so these artifacts are not required to be committed?

🇺🇸United States jcandan

Moved to the source example project driving Drupal modules to use webpack builds.

As I learn more about CKEditor 5 Plugin plugin development, I understand now that projects like Editor Advanced Link are led to implement a build by this project.

I am curious what, if any, dependency issues exist that might prevent a Drupal module from not including a build process for CKEditor Plugin plugins.

🇺🇸United States jcandan

Re-titled this issue to be more clear.

Production build 0.71.5 2024