emulsify-ds / emulsify-drupal

Drupal theme built with Storybook and Webpack
GNU General Public License v2.0
91 stars 42 forks source link

Stable9 theme dependency change #197

Closed ccjjmartin closed 2 years ago

ccjjmartin commented 3 years ago

Need to update documentation and this line to reflect stable9 instead of stable. Sous also needs to be updated to enable the stable9 theme as you get a fatal error by depending upon it but not enabling it.

Review the templates directory I found that we don't have a lot of templates that stable9/stable do, for purposes of scoping this issue I am going to leave out all of the templates that we don't currently have and only focus on reviewing the ones that we do have and are included in stable9:

List of components to be included in this issue:

  1. block.html.twig
  2. node.html.twig
  3. page-title.html.twig
  4. table.html.twig
  5. checkboxes.html.twig
  6. field-multiple-value-form.html.twig
  7. fieldset.html.twig
  8. form-element-label.html.twig
  9. form-element.html.twig
  10. input.html.twig
  11. radios.html.twig
  12. select.html.twig
  13. textarea.html.twig
  14. page.html.twig
  15. status-messages.html.twig
  16. breadcrumb.html.twig
  17. menu-local-action.html.twig
  18. menu-local-task.html.twig
  19. menu-local.tasks.html.twig
  20. menu.html.twig
  21. pager.html.twig
  22. views-view-unformatted--frontpage.html.twig
evanmwillhite commented 3 years ago

This might be more involved than updating this line. At the very least, anything in /templates should be updated to match the corresponding folder structure and file contents in Stable9's theme, but we should also test to ensure it is compatible and doesn't have errors.

ccjjmartin commented 3 years ago

I updated the description to include the templates I thought needed to be review as part of this PR.

Looking over the blocks but not making changes yet I have the following comments:

  1. block.html.twig file I noticed that we replaced {{ attributes }} with {{ bem() }} I know at one point we had attributes working with in Emulsify. Would it not be better to print both the full attributes object without classes and then print classes coming from Drupal with our bem classes tacked on to the end of the list? Then we get even more integration with the Drupal backend?
  2. node.html.twig same as above otherwise looks like nothing changed
  3. page-title.html.twig same as above, we are removing the ability for the backend to render title attributes
  4. table.html.twig interestingly enough here we are adding the class table to the attributes object and not using bem at all. {{ attributes.addClass('table') }} Otherwise, other differences were that we added some bem classes (hard coded) to the table rows but everything else from core is already there.
  5. checkboxes.html.twig - Looks like we have a todo on this to remove some files related to this when a drupal core issue is resolved but it is still marked as active: https://www.drupal.org/node/1819284
  6. field-multiple-value-form.html.twig - same as above form comment
  7. fieldset.html.twig - looks like we added form-fieldset class, we are missing a possible new class "fieldset-legend", we are also missing a span within the legend that has some attributes around the title
  8. form-element-label.html.twig - good to go here, no changes needed
  9. form-element.html.twig - missing some new classes coming from core, needs work
  10. input.html.twig - good to go here, no changes needed
  11. radios.html.twig - good to go here, no changes needed (unless we want to fix the bem class being added form-item--radio this isn't consistent across form items, example form-item__textfield
  12. select.html.twig - we added an extra wrapping div or it went away, {% apply spaceless %} versus what we have {% spaceless %}
  13. textarea.html.twig - good to go here, no changes needed
  14. page.html.twig - we aren't consistent with core at all so I am going to skip reviewing this
  15. status-messages.html.twig - missing a data attribute, aria label needs, some changes, basically this one needs some work
  16. breadcrumb.html.twig - looks good, no changes necessary
  17. menu-local-action.html.twig - wipes out the attributes
  18. menu-local-task.html.twig - not comparable to what Drupal has, we rewrote the tabs component completely
  19. menu-local.tasks.html.twig - not comparable to what Drupal has, we rewrote the tabs component completely
  20. menu.html.twig - not comparable to what Drupal has, we rewrote the tabs component completely
  21. pager.html.twig - needs further review but looks very similar
  22. views-view-unformatted--frontpage.html.twig - not comparable
ModulesUnraveled commented 2 years ago

@ccjjmartin Has this been completed? I thought we did something to update the files... Maybe I'm wrong?

ccjjmartin commented 2 years ago

@ModulesUnraveled Yep, here is the PR: https://github.com/emulsify-ds/emulsify-drupal/pull/210

Guess I didn't close the issue associated with it. Closing.