Theming the default contact form #759
Conversation
There was a problem hiding this comment.
@mgurjanov Looks good, should we disable the contact module now?
|
@mariano-dagostino You stole my question! I was thinking of same. So you agree? |
|
@mariano-dagostino I removed core contact! |
|
@sonvir249 Please take a look. |
web/modules/custom/server_general/src/Plugin/EntityViewBuilder/ParagraphForm.php
Outdated
Show resolved
Hide resolved
| * @return array | ||
| * Render array. | ||
| */ | ||
| public function buildFull(array $build, ParagraphInterface $entity): array { |
There was a problem hiding this comment.
@amitaibu This is what I actually wanted to point out before but didn't put it together well. If we can use smaller trait component methods (FE methods) to build render array step by step, do we need to pass title, body, items etc like we do with other PT PEVB plugins in DS? To me adding another trait method just for this PT or adding a twig doesn't seem needed. Or even using some method like: buildElementLayoutTitleBodyAndItems. Or do we always have to use trait methods only from other trait methods?
There was a problem hiding this comment.
What we should improve here, is that PEVB shouldn't be calling stuff like buildParagraphTitle directly.
Instead it would call a trait with a buildWebform(string $title, array $description = [], array $form = []): array();
By having this buildWebform, we could show it on the style guide.
So it's not about calling traits from other traits. It's about separation of conecerns. PEVB is about extracting the dynamic data, and the FE traits are about getting raw data, and theming it.
Does it answer? :)
There was a problem hiding this comment.
For me it's also about knowing where things come from. So we have server-theme-accordion.html.twig? Then we have AccordionTrait, accordion.pcss, accordion.js etc and that's super easy to navigate with Ctrl+N on phpstorm. Before when we have InnerElementTrait and InnerElementLayoutTrait etc it was super confusing to me at least and also tedious when trying to work out a general overview of how the element is put together.
There was a problem hiding this comment.
Thank you both. I get it now. I was a bit confused with some implementations on other projects where we have not so uniform implementation.
There was a problem hiding this comment.
@mgurjanov no worries, and yes other projects may be confusing because we were still working out how to do this :)
For me personally, I'd prefer "easier to understand/maintain" more than "elegant". So for example, personally I'd prefer doing the layout, styling etc of a piece of text in its own server-theme-element-- twig, rather than have it split over multiple smaller server-theme-inner-element-layout-- etc twigs.
But for much smaller elements like button, responsive-font-size, vertical-spacing etc, it makes sense to have traits that do this, so it's easier to maintain (change one twig and the whole site is updated)
So it's not really exact science, there's lot of arbitration, on my part at least, and I think we'll get there eventually :)
There was a problem hiding this comment.
For me personally, I'd prefer "easier to understand/maintain" more than "elegant"
Same 😄
personally I'd prefer doing the layout, styling etc of a piece of text in its own server-theme-element-- twig, rather than have it split over multiple smaller server-theme-inner-element-layout-- etc twigs.
@bboro can you please give an example, I'm not sure I understand.
There was a problem hiding this comment.
I mean I personally prefer having server-theme-element--awesome-card.html.twig which defines its own grid grid-cols-2 etc layout, than having to compose this through wrapInnerElementLayoutAwesomeCard.
There was a problem hiding this comment.
I see your point; thanks. But I'd still advocate being stingy with creating new Twigs, as I’ve seen it’s all too easy to add custom styles rather than reuse existing ones within the Twig.
So, as we break things down into smaller pieces, the goal is to guide devs away from adding new Twigs unnecessarily. New Twigs are the devil! 🤣
There was a problem hiding this comment.
New Twigs are the devil! 🤣
🤣 Or maybe you can think of them as just an ever growing number of minions. 😄 😄 😄 😄
| $webform = $this->entityTypeManager->getStorage('webform')->load($webform_name); | ||
| if (empty($webform) || !$webform->isOpen()) { | ||
| return []; | ||
| } |
| * @return array | ||
| * Render array. | ||
| */ | ||
| public function buildFull(array $build, ParagraphInterface $entity): array { |
There was a problem hiding this comment.
What we should improve here, is that PEVB shouldn't be calling stuff like buildParagraphTitle directly.
Instead it would call a trait with a buildWebform(string $title, array $description = [], array $form = []): array();
By having this buildWebform, we could show it on the style guide.
So it's not about calling traits from other traits. It's about separation of conecerns. PEVB is about extracting the dynamic data, and the FE traits are about getting raw data, and theming it.
Does it answer? :)
#726