DP-5234/DP-5177 Refine and Refactor Patterns #571
Conversation
legostud
changed the title
There was a problem hiding this comment.
@legostud - lots of great work here! I do have some questions, mostly questioning / confirming backwards compatibility since we're targeting a minor release with these changes.
Thank you!
| title="{{ button.info }}"> | ||
| {{ button.text }} | ||
| </a> | ||
| title="{{ button.info }}">{{ button.text }}</a> |
There was a problem hiding this comment.
I wonder if the spaceless tag would be useful for situations like this as well?
There was a problem hiding this comment.
I didn't know there was a spaceless tag. Good to know.
| @@ -2,4 +2,4 @@ | |||
| This is a variant of the [Cat Icon](./?p=atoms-cat-icon) pattern showing it is in a smaller size. | |||
|
|
|||
| ### How to generate | |||
| * set the "small" variable to "true" | |||
| * set the `small` variable to "true" | |||
There was a problem hiding this comment.
Based on the .json and .md file, I think this should be true (boolean) vs "true" (string) - right?
| {% set smallClass = catIcon.small ? "ma__category-icon--small" : "" %} | ||
|
|
||
| {# Backward compatible with Release5.7.0 where small was a string value #} | ||
| {% if catIcon.small is same as ("false") %} |
There was a problem hiding this comment.
For my own learning, this is because a non-empty string is truthy, correct?
There was a problem hiding this comment.
yes, "" is a truthy value. Same as "false" is also truthy.
| {% endfor %} | ||
| </div> | ||
| {% set formDownloads = { "downloadLinks": actionStep.downloadLinks } %} | ||
| {% include "@organisms/by-author/form-downloads.twig" %} |
There was a problem hiding this comment.
Is this a backwards compatible change? I didn't see any other occurrences of __downloads does that mean the css is still available to style both markup versions until the next major release? Should we start a list of changes like this? Or add a comment in the pattern .scss file?
There was a problem hiding this comment.
comment already existing in the scss file
&__downloads {
margin-bottom: 1em; // for v5.6 and below
| <a href="{{ backButton.link.href }}" aria-label="{{ backButton.link.info }}">{{ backButton.link.text }}</a> | ||
| {% set button = backButton.button %} | ||
|
|
||
| {# backward compatible with v5.6 - swapped custom link with a button #} |
| {% include "@organisms/by-template/jump-links.twig" %} | ||
| {% for stackedRowSection in stackedRowSections %} | ||
| {% include "@organisms/by-author/stacked-row-section.twig" %} | ||
| {% endfor %} |
There was a problem hiding this comment.
Just confirming that this is a non-breaking change because we're extending stacked-row-template which still accepts stackedRowSections - correct?
| @@ -33,7 +33,7 @@ emergencyAlerts: { | |||
| emergencyHeader: { | |||
| type: emergencyHeader / required | |||
| }, | |||
| alerts: [{ | |||
| alerts (optional): [{ | |||
| <button class="ma__button-minor ma__button-minor--small" onClick="window.history.back();" type="button">{{ transitionPage.back }}</button> | ||
| <button class="ma__button ma__button--small" type="submit">{{ transitionPage.submit }}</button> | ||
| <button class="ma__button ma__button--minor ma__button--small" onClick="window.history.back();" type="button">{{ transitionPage.back }}</button> | ||
| {# backward compatible with v5.6 - check for old 'submit' variable #} |
| @@ -44,8 +44,6 @@ | |||
| {% include "@atoms/09-media/image.twig" %} | |||
| {% endif %} | |||
| {% if introSidebar.social %} | |||
| {% set sidebarHeading = introSidebar.social.sidebarHeading %} | |||
| {% include "@atoms/04-headings/sidebar-heading.twig" %} | |||
There was a problem hiding this comment.
For my own learning, this works because we've added iconLinks.compHeading which has a sub property to render as an old sidebar heading - correct?
Is it backwards compatible if we send the compHeading outside of social links as we likely are in massgov now?
There was a problem hiding this comment.
It works, because we added iconLinks.compHeading and the .sidebar has CSS that renders Comp Headings like Sidebar Headings.
There was a problem hiding this comment.
and no it's not backwards compatible if the twig changes prior to the data layer
There was a problem hiding this comment.
Thanks for adding that backwards compatible logic!
| {% block preContent %} | ||
| {% include "@organisms/by-author/search-banner.twig" %} |
There was a problem hiding this comment.
Does this change the data structure in a way that's backwards compatible?
| title="{{ button.info }}"> | ||
| {{ button.text }} | ||
| </a> | ||
| title="{{ button.info }}">{{ button.text }}</a> |
There was a problem hiding this comment.
I didn't know there was a spaceless tag. Good to know.
| @@ -2,4 +2,4 @@ | |||
| This is a variant of the [Cat Icon](./?p=atoms-cat-icon) pattern showing it is in a smaller size. | |||
|
|
|||
| ### How to generate | |||
| * set the "small" variable to "true" | |||
| * set the `small` variable to "true" | |||
| {% set smallClass = catIcon.small ? "ma__category-icon--small" : "" %} | ||
|
|
||
| {# Backward compatible with Release5.7.0 where small was a string value #} | ||
| {% if catIcon.small is same as ("false") %} |
There was a problem hiding this comment.
yes, "" is a truthy value. Same as "false" is also truthy.
| {% endfor %} | ||
| </div> | ||
| {% set formDownloads = { "downloadLinks": actionStep.downloadLinks } %} | ||
| {% include "@organisms/by-author/form-downloads.twig" %} |
There was a problem hiding this comment.
comment already existing in the scss file
&__downloads {
margin-bottom: 1em; // for v5.6 and below
| <div class="ma__banner-credit"> | ||
| <div class="ma__banner-credit__container"> | ||
| <div class="ma__banner-credit__icon"> | ||
| {% include "@atoms/05-icons/svg-marker-outline.twig" %} |
| {% include "@molecules/page-intro.twig" %} | ||
|
|
||
| {% set searchBannerForm = searchBanner.searchBannerForm %} | ||
| {% include "@molecules/search-banner-form.twig" %} |
There was a problem hiding this comment.
hmm, I think I made an assumption here that the twig and variables would be updated at the same time which may not be the case for mass.gov.
| @@ -44,8 +44,6 @@ | |||
| {% include "@atoms/09-media/image.twig" %} | |||
| {% endif %} | |||
| {% if introSidebar.social %} | |||
| {% set sidebarHeading = introSidebar.social.sidebarHeading %} | |||
| {% include "@atoms/04-headings/sidebar-heading.twig" %} | |||
There was a problem hiding this comment.
It works, because we added iconLinks.compHeading and the .sidebar has CSS that renders Comp Headings like Sidebar Headings.
| @@ -44,8 +44,6 @@ | |||
| {% include "@atoms/09-media/image.twig" %} | |||
| {% endif %} | |||
| {% if introSidebar.social %} | |||
| {% set sidebarHeading = introSidebar.social.sidebarHeading %} | |||
| {% include "@atoms/04-headings/sidebar-heading.twig" %} | |||
There was a problem hiding this comment.
and no it's not backwards compatible if the twig changes prior to the data layer
| @@ -3,5 +3,5 @@ | |||
| {% block narrowContent %} | |||
| {% include "@organisms/by-template/error-page.twig" %} | |||
| {% include "@molecules/header-search.twig" %} | |||
| {% include "@organisms/by-author/helpful-links.twig" %} | |||
| {% include "@organisms/by-author/link-list.twig" %} | |||
There was a problem hiding this comment.
In this case we don't need it to be Backward Compatible. Pages are never consumed only reviewed as examples.
| @@ -3,5 +3,5 @@ | |||
| {% block narrowContent %} | |||
| {% include "@organisms/by-template/error-page.twig" %} | |||
| {% include "@molecules/header-search.twig" %} | |||
| {% include "@organisms/by-author/helpful-links.twig" %} | |||
| {% include "@organisms/by-author/link-list.twig" %} | |||
There was a problem hiding this comment.
Pages are just examples and as such don't need to be backward compatible.
There was a problem hiding this comment.
@legostud awesome work! I think you've addressed everything! Thanks for going back through and adding some more backwards compatible logic - I appreciate it! I think this is ready to be merged!
Description
Several patterns have been updated to make them more modular by including other patterns, more flexible by adding options, or more maintainable by using proper variables, the documentation has been updated to use ` instead of ' for variable names, and unused Patterns were tagged as obsolete.
Related Issue / Ticket
https://jira.state.ma.us/browse/DP-5234
Steps to Test
Impacted Areas in Application
smallvariable to type booleanawith link atomh2for the title.Notes
Today I learned...