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

Issue #3415738 by joshua1234511, alex.skrypnyk: Decommissioned the 'Quote' standalone component. #1221

Merged
merged 11 commits into from
Mar 12, 2024

Conversation

joshua-salsadigital
Copy link
Collaborator

@joshua-salsadigital joshua-salsadigital commented Mar 7, 2024

https://www.drupal.org/project/civictheme/issues/3415738

Checklist before requesting a review

  • I have formatted the subject to include ticket number as [CS-123] Verb in past tense with dot at the end.
  • I have added a link to the issue tracker
  • 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. Removed Quote component: configuration, template, preprocess.
  2. Created an update hook to convert a quote component to a content component.

Screenshots

@joshua-salsadigital joshua-salsadigital added the State: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request label Mar 7, 2024
@joshua-salsadigital joshua-salsadigital self-assigned this Mar 7, 2024
@joshua-salsadigital joshua-salsadigital force-pushed the feature/3415738-remove-standalone-component branch from 96c61cb to 2b3d1e1 Compare March 7, 2024 16:07
Copy link
Contributor

@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
This looks great! Thank you for working on this!

It misses 2 parts:

  1. We need to update the fixture DBs using CT 1.3 to contain Quote component (in case it does not), so that we could have a test for the update hook. I know this is a tedious task, but it is something that we really need to make sure that the updates work as expected.
  2. I realised that <cite> is something that would need to be added manually in the CKeditor - can we use some sort of template for this? At least try to see what is available?

@joshua-salsadigital
Copy link
Collaborator Author

@joshua-salsadigital This looks great! Thank you for working on this!

It misses 2 parts:

  1. We need to update the fixture DBs using CT 1.3 to contain Quote component (in case it does not), so that we could have a test for the update hook. I know this is a tedious task, but it is something that we really need to make sure that the updates work as expected.
  2. I realised that <cite> is something that would need to be added manually in the CKeditor - can we use some sort of template for this? At least try to see what is available?

1: Having issues with build, will try again. Not able to get it working with older civic theme checkout to 1.3
2: One option is to add a cite plugin custom into civic theme.

@AlexSkrypnyk
Copy link
Contributor

@joshua-salsadigital

1: Having issues with build, will try again. Not able to get it working with older civic theme checkout to 1.3

Could you please provide some details on this. If 1.3 does not work - let's update those fixtures to 1.4.

2: One option is to add a cite plugin custom into civic theme.
I will create a new ticket for this. Let's ignore this for now.

@joshua-salsadigital
Copy link
Collaborator Author

1.4

Issue with 1.3 is missing quote component,

  • tried to cherry pick and get the component in 1.3 but there is no smooth way to do it, build fails as major change in components took place.
    Using 1.4 we are loosing test for renamed components as they have already been updated and test they will be skipped.

@AlexSkrypnyk
Copy link
Contributor

@joshua-salsadigital
It is okay that tests will be skipped for 1.3

Please feel free to update and/or remove those tests. Also, no need to keep 1.3 dump around - just make a dump of 1.4 and commit that.

@joshua-salsadigital joshua-salsadigital force-pushed the feature/3415738-remove-standalone-component branch from 64baf38 to 11f4d20 Compare March 12, 2024 09:26
@joshua-salsadigital joshua-salsadigital added State: Needs review Pull requests needs a review from assigned developers and removed State: Requires more work Pull request was reviewed and reviver(s) asked to work further on the pull request labels Mar 12, 2024
@AlexSkrypnyk AlexSkrypnyk changed the title Issue #3415738] by joshua1234511: Decommissioned the 'Quote' standalone component. Issue #3415738 by joshua1234511, alex.skrypnyk: Decommissioned the 'Quote' standalone component. Mar 12, 2024
@AlexSkrypnyk AlexSkrypnyk merged commit a430f3f into develop Mar 12, 2024
12 checks passed
@AlexSkrypnyk AlexSkrypnyk deleted the feature/3415738-remove-standalone-component branch March 12, 2024 16:41
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
Development

Successfully merging this pull request may close these issues.

2 participants