Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIVIC-1508] Fixed CKEditor 5 Paragraphs overal in Claro. #1131

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

joshua-salsadigital
Copy link
Collaborator

@joshua-salsadigital joshua-salsadigital commented Oct 31, 2023

https://salsadigital.atlassian.net/browse/CIVIC-1508

Checklist before requesting a review

  • I have formatted the subject to include ticket number as [CIVIC-123] Verb in past tense with dot at the end.
  • I have added a link to the JIRA ticket
  • I have provided information in Changed section about WHY something was done if this was not a normal implementation
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • I have run new and existing relevant tests locally with my changes, and they passed
  • I have provided screenshots, where applicable

Changed

  1. [CIVIC-1508] Fixed CKEditor 5 Paragraphs overal in Claro. #1131 (comment)

Screenshots

Screenshot 2023-10-31 at 10 50 26 PM
Screenshot 2023-10-31 at 10 50 59 PM

@joshua-salsadigital joshua-salsadigital added the State: Needs review Pull requests needs a review from assigned developers label Oct 31, 2023
@joshua-salsadigital joshua-salsadigital self-assigned this Oct 31, 2023
@joshua-salsadigital
Copy link
Collaborator Author

joshua-salsadigital commented Oct 31, 2023

Investigated the issue.
Found relevant issues on D.O.
https://www.drupal.org/project/paragraphs_browser/issues/3332058
https://www.drupal.org/project/paragraphs/issues/3112245
https://www.drupal.org/project/paragraphs/issues/3375611
https://www.drupal.org/project/drupal/issues/3145188
https://www.drupal.org/project/drupal/issues/3372720

On reading through the issues.
Stand alone Carlo does not replicate the issue
Stand alone Paragraph does not replicate the issue

Issue is due to CKEditor 5

The issue is not RTBC due to decision to made on where the fix should be. The mostly likely decision will be to get it applied to carlo to target CKEditor 5 in core.

Based on the decision Have applied the Working Patch : https://www.drupal.org/project/drupal/issues/3332416

@AlexSkrypnyk AlexSkrypnyk requested a deployment to PR October 31, 2023 18:14 Abandoned
Copy link
Collaborator

@AlexSkrypnyk AlexSkrypnyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshua-salsadigital
Thank you for finding a solution.

The implementation in this PR will not work: it patches the source dev site and it is not bundled with CivicTheme theme or modules.

Can you please take the solution from the patch and add to the civictheme_admin module. Could you please do it in such way so that when this patch is accepted by core, our override in the module would not be conflicting.

@joshua-salsadigital
Copy link
Collaborator Author

Updated the civictheme_admin to include the solution. Added to exixting claro-node-form-overrides as both are form node reacted updates.

@AlexSkrypnyk AlexSkrypnyk requested a deployment to PR November 1, 2023 03:05 Abandoned
@AlexSkrypnyk
Copy link
Collaborator

@joshua-salsadigital
this is excellent! thank you

@AlexSkrypnyk AlexSkrypnyk merged commit 6808d45 into develop Nov 1, 2023
9 checks passed
@AlexSkrypnyk AlexSkrypnyk deleted the feature/CIVIC-1508 branch November 1, 2023 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
State: Needs review Pull requests needs a review from assigned developers
Projects
None yet
2 participants