emulsify-ds / compound

Compound is the default component collection for Emulsify
https://emulsify-ds.github.io/compound/
GNU General Public License v2.0
12 stars 12 forks source link

#57 Dependency Improvements & Refactoring & Renaming #61

Closed emircanerkul closed 5 months ago

emircanerkul commented 1 year ago

Improvements are done. Now all components can work as individual. All existing components are ported to this concept.

Also, there are other changes in other repositories. To be able to test and apply this PR other PR's also must be used.

https://github.com/emulsify-ds/emulsify-cli/pull/173 https://github.com/emulsify-ds/emulsify-drupal/pull/269

There are a couple of patterns used across all components. To share this knowledge with the team I'm also documenting it here.

image

Naming Patterns:

[component_name].component.scss: This pattern allow file to be compiled individually. [component_name].mixin.scss: This pattern allows files to be used across all files. Do not add anything that create output. Only use for mixin or variables. libraries.yml: Classic drupal library definition file. These file will be explored by Drupal.

Library naming should be in harmony with the file path. atoms.links.link | atoms.text.headings | molecules.pager | molecules.menus.social | organisms.site.site_footer etc.

Usage within the Twig

Just attached what is needed. Do not attach any child dependencies. They are automatically imported when we use the same pattern.

{{ attach_library('emulsify/organisms.site.site_footer') }}

Usage within the Storybook

Styles must be included individually in the **.stories.js files. import './site-header/_site-header.component.scss';

Further Ideas and Todos:

Thx!

ModulesUnraveled commented 1 year ago

Wow. This is a huge rewrite. I'll take a look at this soon, but because it's so large, I'm not sure how quickly we'll be able to review it all. Thank you for your contribution!

At first glance, I'm intrigued by some of your organization recommendations, but also not sure everything follows our philosophy.

I hope to get this reviewed soon. Thanks again!

emircanerkul commented 1 year ago

Wow. This is a huge rewrite. I'll take a look at this soon, but because it's so large, I'm not sure how quickly we'll be able to review it all. Thank you for your contribution!

At first glance, I'm intrigued by some of your organization recommendations, but also not sure everything follows our philosophy.

I hope to get this reviewed soon. Thanks again!

Yep, Good luck :) and Thank you.

I don't exactly know what emulsify's philosophy but if emulsify is a component system, component isolation/optimization/separation should be there because it's a component as it was written.

My main Idea and Point in this pr is Library separation and dependency management. We may want to create a huge website that has lots of organism variety. In Drupal, all elements/organisms/even atoms (which we might not use form atoms page to page) should not create one huge CSS file. With this dependency management, all headache is solved. Also with this technique, we have a chance to eliminate system.emulsify.json file.

Thanks to new web techs, multiple files are no problem anymore (HTTP2). But In addition to that Drupal also merge all styles that are used on the page and serves page-specific manner (according to my observations).

Historically, one of the main advantages x in having a single CSS file is the speed benefit when using HTTP1.1. However, as of March 2018 over 80% of browsers now support HTTP2 which allows the browser to download multiple resources simultaneously as well as being able to push resources pre-emptively. Having a single CSS file for all pages means a larger than necessary file size. With proper design, I don't see any advantage in doing this other than its easier to code.

The ideal design for HTTP2 for best performance would be:

Have a core CSS file which contains common styles used across all pages. Have page specific CSS in a separate file Use HTTP2 push CSS to minimise wait time (a cookie can be used to prevent repeated pushes) Optionally separate above the fold CSS and push this first and load the remaining CSS later (useful for low-bandwidth mobile devices) You could also load remaining CSS for the site or specific pages after the page has loaded if you want to speed up future page loads.

https://stackoverflow.com/a/49397312

ModulesUnraveled commented 1 year ago

Are you available to join our community meetup at any time? We meet on the first Thursday of each month at 1pm Central (I think that's 9pm in Turkey).

One of my concerns with this is that it's very Drupal-specific. While we do a lot of Drupal work, our Components support Wordpress, and potentially other frameworks. So, we try to keep anything Drupal-specific (like libraries.yml files) out of Compound etc.

One other thing is that the system.emulsify.json file is built to support our CLI which enables organizations to share a common component library across projects. You can easily install an organization component into each project as needed. So, we wouldn't want to get rid of that, because we'd lose some vital functionality.

Anyway, we have an Emulsify Slack (which you can join here: https://launchpass.com/emulsify) and we meet once a month as a community. If you could come to one of those, I'd love to get a walk-through of the ideas you've presented here, and see if there are parts that we want to include in "core".

emircanerkul commented 1 year ago

@ModulesUnraveled yes sure.

Firstly I want to clear some points. I am aware of cli and its advantage. I do not want to remove this functionality. It's really useful.

Let's think first about why we need the system.emulsify.json file. It's obvious to say give information to emulsify cli to perform automatic imports. So after the individual libraries.yml file approach, we can grab the same information from these files, and under the hood, we can dynamically build this system.emulsify.json file. With this approach, we are just trying to eliminate duplicate work.

My other point; YAML files are not especially related to drupal. YAML files are just little files that contain component information individually in this approach.

We can change file names even if we also can use a JSON file. But I selected the YAML file because the helper function is already developed for drupal and drupal's libraries discovery work with YAML.

I think All bricks come together. If any more are left we also can discuss them in slack.

ModulesUnraveled commented 1 year ago

@emircanerkul Thanks again for this, there's a lot of work in here that we really like.

However, it's such a large PR that it's hard to review all of the parts at once. We discussed this in our weekly checkin, and would like to ask if you can break this into separate PRs that are more focused on incremental changes, rather than globbing it all into one giant PR.

For example:

emircanerkul commented 1 year ago

@ModulesUnraveled I'm unsure if I can split to multiple PRs because all depend on each other, including PRs that I sent in other repos. But lots of parts in this pr follow a similar pattern.

Actually, individual libraries are the main point of this PR. To create a dependency chain, it's required for webpack and drupal. All component's assets are compiled and included as individuals. The Drupal library system grabs each required one and optimizes the output. Only required component's assets are included on the drupal page.

image
emircanerkul commented 1 year ago

Anyway, I am also not a fan of twig. Currently, I am involved in js technologies these days. SvelteKit is just a really good one. With vite it's really speedy and lots of capabilities they have. All optimization and hassle-free configs are really great to work with unlike webpack and twig and library dependency things...

Might be my work in this repo is for extreme cases and performance and optimization things. But now vite and sveltekit's component system and encapsulation are already placed and works perfect.

I think in drupal we just need to make entity management (structural content) and API's perfect (Just run as a headless one). Otherwise, we can't take any advantage of other works in open source (huge js ecosystem especially) and it seems not wise to me.

BTW, feel free to close my all PRs. Without dependency separation, this also works. And on these days I think no one cares about an extra 10kb in the assets file. Let's say 2 MB in a huge project. It's still not a big deal.

Thank you for all.

josue2591 commented 5 months ago

@callinmullaney due to the phase we are with this project I think we can close this PR