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

[emulsify-ds/compound/issues/57] Dependency Improvements & Refactoring & Renaming #269

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

emircanerkul
Copy link

@emircanerkul emircanerkul commented Sep 16, 2022

Dependency Improvements & Refactoring & Renaming

With this PR, All components work as individuals. To accomplish that some hooks are written and webpack configs have been changed. Drupal automatically explores all sub-libraries.

Libraries in the global file are moved to their own files.

Dist folder output: All files are individually exported

image

┆Issue is synchronized with this Clickup by Unito

@@ -4,3 +4,105 @@
* @file
* Functions to support theming.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

This might make more sense to go in the Emulsify Twig module rather than in each theme. If it's going to be the recommended way to write libraries, I'd hate for someone to not know what this is, and delete it from their theme, and then ask us why their libraries don't work... If it's in the module (which is a Drupal dependency) they can't delete it, and keeps "required" code out of their custom codebase.

Copy link
Author

Choose a reason for hiding this comment

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

To do that, we also need to change the code a little bit. (Need to get the active theme and dir)

$theme_name = basename(__FILE__, '.theme'); // compony

Or we can just wait for this feature to become included in the core https://www.drupal.org/project/drupal/issues/3092496

We can propagate the idea of If you don't know and if all works good, DON'T touch anything! 😄

@callinmullaney
Copy link
Collaborator

@emircanerkul Thank you for submitting this work. I did a light functional test on this and the functionality you are adding is great! I think this is likely the direction we would like to take emulsify as it allows components to work a bit more independently and removes a manual step we all need to do with Drupal. Creating a drupal library to attach somewhere.
I'm in somewhat of an agreement with ModulesUnraveled assessment that this code would be better managed from a module or maybe even a base theme. Because Emulsify is a starterkit for custom theme's this means this large php function would need to carry over. Also, we would need to add some extra tooling to the emulsify to check php formatting and Drupal deprecation's if we wanted to support this code long-term.
The issue you linked has a related issue that takes a different approach and is seeing a bit more traction. I'm keeping an eye on this one as I would definitely like to see this functionality implemented somewhere on this project: https://www.drupal.org/project/drupal/issues/3075427
That being said, this feature as well as several common preprocessing functions have made the rounds in our internal discussions. Their might be an opportunity here to introduce a base theme where we could contain this functionality in a more isolated space but still see it's benefits. I'm going to place this PR on hold for now and update it with a path forward if the linked issue above is merged or a base theme is being developed.
Thanks again for all your efforts!

@emircanerkul
Copy link
Author

@callinmullaney thanks to you too.

@callinmullaney callinmullaney marked this pull request as draft July 26, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

3 participants