search

localgovdrupal / localgov_campaigns

⛔️ DEPRECATED (Provides campaign sections using page builder paragraph types for LocalGov Drupal.)
GNU General Public License v2.0
0 stars 2 forks source link

Added Accordion paragraph #52

Closed gosia-mlynarczyk closed 4 years ago

gosia-mlynarczyk commented 4 years ago

Added 'Accordion' paragraph (and its child 'Accordion pane' paragraph).

Since this is my first pull request I'd appreciate a thorough review and feedback 🙂

gosia-mlynarczyk commented 4 years ago

Thanks @stephen-cox , that's very helpful!

Yeah, I wasn't sure about naming convention for fields, as some of them are prefixed with field_ and some with localgov_. I'll change it.

Re: headings & heading levels, there's actually a module (https://www.drupal.org/project/heading) which provides a 'heading' field type. So rather than adding a 'title' and 'heading level' fields to paragraphs we would only need to add 1 'heading' field. Do you think it would be better to use that module instead?

stephen-cox commented 4 years ago

Re: headings & heading levels, there's actually a module (https://www.drupal.org/project/heading) which provides a 'heading' field type. So rather than adding a 'title' and 'heading level' fields to paragraphs we would only need to add 1 'heading' field. Do you think it would be better to use that module instead?

It seems a simple thing to have a two fields, title and heading level, but if the module provides extra benefits then it's fine to use it. It does provide an extra dependency that we have to manage, so we need to weigh the benefits against the extra work with upgrades and other potentially breaking changes introduced by contrib modules.

I'm happy with the field structure as it stands. My query was really over what the benefits are of using the _create_heading function to process the title and heading level were over doing all the display stuff in the paragraph--localgov-accordion-pane.html.twig template file.

gosia-mlynarczyk commented 4 years ago

Ok, I'll leave those 2 separate fields then rather than using a contrib module.

The reason I created a custom _create_heading function is that there will be more paragraphs using title / heading level combo. But I could just create that heading element in each paragraph's template.

stephen-cox commented 4 years ago

The reason I created a custom _create_heading function is that there will be more paragraphs using title / heading level combo. But I could just create that heading element in each paragraph's template.

That's fine. Just wondering why you took that approach.

If you rename the fields and fix the coding standards error I'm happy to approve this merge request.