Account created on 29 January 2020, over 5 years ago
  • Engineer - Frontend at QED42 
#

Merge Requests

More

Recent comments

🇮🇳India meghasharma

I pushed the changes to the Drupal core branch and updated the issue status to 'Needs review'.

🇮🇳India meghasharma

I’ve assigned this issue to myself. I initially made the changes in the XB core's user.schema.yml, but later realized that the changes should go into Drupal core. I’d appreciate some guidance on how to proceed and push the changes to the correct branch.

🇮🇳India meghasharma

Added ConfigExists constraint to user_role condition schema

🇮🇳India meghasharma

Hi @hooroomoo,
Thanks for the info. I tried the following in useSourceCode.ts:
Added deep comparison for props and slots using lodash/isEqual.
Updated refs to track changes in props and slots.
Triggered auto-save when props, slots, or source code change.
Verified dispatching compiled JS and CSS updates.

But the issue with auto-save losing changes on quick navigation still happens.

🇮🇳India meghasharma

Pushed the fix for BlockComponent::validateComponentInput() . Removed the early return to ensure input is properly validated. Let me know if anything else is needed.
Test case not included in the MR. Would appreciate if someone more familiar with writing tests could help add coverage for this. Thanks!

🇮🇳India meghasharma

I’d like to start working on a fix, but I couldn’t locate the file where the auto-save logic should be added for props or slots updates. Could you please point me to the right place to make this change? Thank you!

🇮🇳India meghasharma

I’m able to reproduce the issue — when I update props or slots under the Component Data tab without making any changes in the JS or CSS tabs, the updates are not auto-saved.

🇮🇳India meghasharma

I updated the SdcPropKeysConstraintValidator::validate() method to check for extra keys in the input mapping that are not defined in the SDC props.
To support this, I added a new extraMessage property to the SdcPropKeysConstraint class for the error message.

🇮🇳India meghasharma

Just to confirm, can I rename formTemporaryRemoveThisExclamationExclamationExclamation() to buildPropFormElement()?
Also, since massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation() is marked as dead, should I safely remove it?
Please let me know before I proceed. Thanks!

🇮🇳India meghasharma

#4 is still the issue
After making any changes to the image field via the Page Data panel
For example:-
Uploading a new image
Add new alternative text

...the "Review X changes" button at the top still shows “No changes”.
Ideally, these actions should trigger the button to update and show "Review 1 change"

🇮🇳India meghasharma

Verified #3, the read only issue resolved, Now if remove the existing image and uploading new one, the Alternative text field works as expected and is editable.

🇮🇳India meghasharma

Spacing is now correctly added between the "Remove" button and the image preview in the image component.
.field--widget-media-library-widget is now used instead of the field name class — aligns better with field type targeting as suggested.
Attaching before and after screenshots after applying the MR.

🇮🇳India meghasharma

I’ve created a merge request that removes the `_permission: 'administer themes'` requirement from the `experience_builder.api.config.list` route, based on Wim Leers’ suggestion in comment #10.
This allows content editors with the proper `xb_page` permissions to view and place components without needing the `administer themes` permission.
The route is still protected by `_user_is_logged_in: 'TRUE'` and per-entity `access('view')` checks, so there are no security risks introduced.

Tested and confirmed:
Before fix: Component list was not visible for content editors.
After fix: Component list is visible and components can be placed.

Attaching screenshots of before and after the fix for reference.

🇮🇳India meghasharma

meghasharma made their first commit to this issue’s fork.

🇮🇳India meghasharma

I was able to reproduce this issue as described.
Steps followed:

- Created a code component named "myCode".
- Added it to the component list.
- Deleted the "myCode" component from the list.
- Tried to create a new code component with the same name "myCode".

The error message appears: "Cannot read properties of undefined (reading 'startsWith')" and the creation fails.

This seems like a bug in how deleted component names are handled in memory or state.

🇮🇳India meghasharma

Additional observation while reproducing the issue:
After making any changes to the image field via the Page Data panel — for example:
Uploading a new image
Removing the image

...the "Review X changes" button at the top still shows “No changes”.
Ideally, these actions should trigger the button to update and show "Review 1 change"

🇮🇳India meghasharma

I was able to reproduce this issue.
When editing an Article content's Experience Builder Page Data panel:

- If the image is not changed, the Alternative Text field works as expected and is editable.
- But when I remove the existing image and upload a new one, the Alternative Text field becomes non-editable — it shows up, but I cannot enter or type anything in it. No characters appear.

🇮🇳India meghasharma

Thanks @penyaskito for the clarification!
Yes, the client-side validation is now working correctly and shows an error when the alias does not start with /.
Since the publishing behavior is related to #3521759 and not this issue, I’m moving this back to Needs review.

🇮🇳India meghasharma

Updated the patch to add client-side validation to ensure alias starts with /.
However, currently we’re still able to publish even if the alias doesn’t start with /
Attaching screenshot to show the client-side error.

🇮🇳India meghasharma

I was able to reproduce the issue on my local setup. When switching between pages using the navigator, the canvas preview incorrectly displays "Untitled page", even though the correct title appears in the right-hand panel.

After applying the merge request, the canvas preview now correctly shows the updated page title when switching between pages.

Attaching screenshots for both before and after the fix for reference.

🇮🇳India meghasharma

Reopening this issue because issue added a hard-requirement on JsonSchema/Validator.
Getting error while clearing cache.

🇮🇳India meghasharma

Right now I’ve added backend validation to ensure alias starts with /, which resolves the layout error.
Also noticed that the error message “The alias path has to start with a slash” is already being shown in the UI if / is missing — but the page still gets saved.
Should I go ahead and update the frontend logic to block submission if the alias is invalid?
Attaching the screenshot.

🇮🇳India meghasharma

Thanks @akhil for the input!
You're right — adding frontend validation makes sense for better UX and to prevent invalid input early on. I'll update the PR to include this validation inside validateNewValue().

Also, I think we can keep the backend fallback (str_starts_with) check just to be safe in case any invalid alias somehow bypasses the UI (e.g., via API or future form changes).

Let me know if you'd prefer removing the backend fallback.

🇮🇳India meghasharma

I have verified the changes related to renaming SdcPropToFieldTypePropMatcher to JsonSchemaFieldInstanceMatcher and SdcPropJsonSchemaType to JsonSchemaType.
1) Old class names (SdcPropToFieldTypePropMatcher, SdcPropJsonSchemaType) have been completely removed from the codebase.
2) New class names (JsonSchemaFieldInstanceMatcher, JsonSchemaType) are being used consistently throughout the module.
Everything looks good

🇮🇳India meghasharma

Added comments explaining why the properties are protected to support DependencySerializationTrait.

🇮🇳India meghasharma

#22 Yes, because Install and set Claro as the admin theme in XbTestSetup changes already merged in 0.x main branch. may it's done in other issue.

🇮🇳India meghasharma

Rebased the branch, yes there are changes already merged in 0.x
only needed to do e2e tests we no longer need to manually enable claro, have done in MR
Please review

🇮🇳India meghasharma

Rebased the branch and setting the admin theme to Claro came as incoming changes.
please check

🇮🇳India meghasharma

meghasharma changed the visibility of the branch 3514551-consider-enabling-claro to active.

🇮🇳India meghasharma

meghasharma changed the visibility of the branch 3514551-consider-enabling-claro to hidden.

🇮🇳India meghasharma

meghasharma changed the visibility of the branch 3514551-consider-enabling-claro to hidden.

🇮🇳India meghasharma

I tested this issue on my local setup by following the steps mentioned in the issue description. After duplicating the "Test Article" page using the navigator, the new page titled "Test Article (Copy)" was created successfully. In the right-hand editor panel, the Title field was correctly populated with "Test Article (Copy)" and did not appear blank.
Attaching a screenshot for reference.

🇮🇳India meghasharma

I read the issue and noticed inconsistent component names. It would be better to standardize the naming convention. Once we decide on the format, I'll update the names.

🇮🇳India meghasharma

@larowlan, I think we should move the pageDataExists check inside as it's more specific to page data forms.
I will update the MR with this change. Please confirm if this approach looks good.

🇮🇳India meghasharma

Updated the error messages in ErrorCodesEnum::getMessage() as per feedback from @tedbow.

🇮🇳India meghasharma

I've updated the messages in \Drupal\experience_builder\Controller\ErrorCodesEnum::getMessage() to make them more helpful and actionable.

🇮🇳India meghasharma

The PHPStan error regarding the $prop_source parameter still needs to be fixed.

🇮🇳India meghasharma

Updated the regex to apply case-insensitivity only to the file extension part using (?i:...) instead of applying it globally.

🇮🇳India meghasharma

Updated the regex for image-uri and stream-wrapper-image-uri to be case-insensitive using the (?i) flag.
This allows image extensions like .JPG, .PNG, etc., from Media Library to work correctly and fixes the Twig rendering error.
Please review, Let me know if there’s a better approach we should consider.
Thanks @dhruv.mittal for the helpful suggestion!

🇮🇳India meghasharma

Converted all ConfigEntityType plugins to use PHP 8 attributes instead of annotations.

🇮🇳India meghasharma

Renamed `PropSourceComponent.field_data` to `.propSources`.

🇮🇳India meghasharma

Renamed state, prop and component data attributes.

🇮🇳India meghasharma

Added a "Components" tab under the Appearance section and updated the "Enabled Components" and "Disabled Components" tabs under the "Components" section by adding the parent_id., Moved "Page builder components" listing in Structure to "Components" in Appearance.

🇮🇳India meghasharma

Changes Done:
Updated Permission: experience_builder.component.status now uses the administer themes permission.
Updated Links: Changed /admin/structure/component to /admin/appearance/component in component entity definition links.
Removed "Page Builder Components" from Structure.
Please review

Now:
You can see the list of enabled and disabled components at /admin/appearance/component.

Remaining Task:
Add a "Components" tab under the Appearance section.

🇮🇳India meghasharma

Could you please add some more details to the issue description? It would be helpful to have more clarity on the expected behavior and a proposed solution.

🇮🇳India meghasharma

meghasharma made their first commit to this issue’s fork.

🇮🇳India meghasharma

Fixed the issue where long component names were overflowing the badge. Now, the text stays within bounds and displays correctly. Ready for review.

🇮🇳India meghasharma

This is already implemented in the file. The datetime field’s date and the file_uri field’s url are handled the same way as daterange in tests/src/Kernel/EcosystemSupport/FieldTypeSupportTest.php.

'datetime' => [  
  // 🐛 Core bug: this is the computed equivalent of `value`, should be marked internal.  
  // @todo Give this similar treatment as `daterange` in https://www.drupal.org/project/experience_builder/issues/3512853  
  'date' => FALSE,  
],  
'file_uri' => [  
  // 🐛 Core bug: this is the computed equivalent of `value`, should be marked internal.  
  // @todo Give this similar treatment as `daterange` in https://www.drupal.org/project/experience_builder/issues/3512853  
  'url' => FALSE,  
],  

Let me know if anything else is needed. Thanks!

🇮🇳India meghasharma

Marked 'date' (DateTimeItem) and 'url' (FileUriItem) properties as internal
Created DateTimeItemOverride.php to override DateTimeItem and marked the date property as internal.
Updated FileUriItemOverride.php (already present) to mark the url property as internal.
Please review

🇮🇳India meghasharma

Tried conditionally rendering the div using {initialized &&

...

} instead of just applying display: none, but the issue persists. The console still shows the 'Infinity' warning for height. Keeping the original approach with isFinite checks seems to work.

🇮🇳India meghasharma

Instead of directly manipulating the DOM using document.querySelector and menu.style.display = 'none' (lines 84 and 88), we should use Radix UI’s built-in method (if available) to close the menu.

Can we avoid direct DOM manipulation? Radix UI might change internal class names, which can break the code.
Can we use state for menu visibility? The menu’s visibility should be controlled using useState.

const [open, setOpen] = useState(true); // Control menu state

const closeContextMenu = useCallback(() => {
  setOpen(false); // Close the menu using state
}, []);
🇮🇳India meghasharma

I have updated @typescript-eslint/eslint-plugin and @typescript-eslint/parser to 8.26.0 as per the issue requirement.
Installed versions:
@typescript-eslint/eslint-plugin: 8.26.0
@typescript-eslint/parser: 8.26.0

After updating, I ran npm run lint:eslint and encountered the following error:

🇮🇳India meghasharma

After running npm run lint:eslint, I still see the same warning that was present earlier:
Should we now check if we can update to a newer version of @typescript-eslint to resolve this?

🇮🇳India meghasharma

I have upgraded TypeScript to version 5.6.3 as per the discussion.
Verified with: npx tsc --version and npm list typescript
Ran npm install to ensure dependencies are correctly installed

🇮🇳India meghasharma

Yes @wim leers, This can happen when rect.height * canvasViewPortScale results in Infinity. This usually occurs if rect.height has an extremely large value or if canvasViewPortScale is not properly defined.

🇮🇳India meghasharma

Updated MR to resolve the console error for one and two column components. Let me know if any further adjustments are needed.
Thanks @omkar-pd

Production build 0.71.5 2024