Skip to content

Beta - repair site integration issues#4611

Merged
thisisdano merged 54 commits into
library--developfrom
al-ld-remove-compare
Apr 15, 2022
Merged

Beta - repair site integration issues#4611
thisisdano merged 54 commits into
library--developfrom
al-ld-remove-compare

Conversation

@amyleadem

@amyleadem amyleadem commented Mar 29, 2022

Copy link
Copy Markdown
Contributor

Description

After installing USWDS beta onto USWDS-site, we encountered some build and compilation errors. The issues and resolutions are outlined below.

For reference, this branch has been installed on USWDS-site branch al-beta-integration and is available for demo and testing in USWDS-Site PR #1493

This is part 1/2 for the resolution of Issue 1491
(Part 2/2 can be found in USWDS-Site PR #1493)

File structure:

Errors discovered after running npm run build:html

  • New location of /compare directories cause build errors
    • Resolution: Remove /compare directories - they are no longer needed. We can instead reference the variants in the _includes directly.
  • Favicons assets are missing from build
    • Resolution:
      • Restore favicons directory inside uswds-core
      • Remove the .ico file found in the /js directory
      • Update copy.js to include .ico files in the copy task
  • The naming of components produces a redundant usa- in file names inside /html-templates (ex: usa-usa-accordion)
    • Resolution: Update file name output in webpack.twig.config.js
    • ⚠️ Note: This solution does remove the automated prefix across all built html files in /html-templates; eg. all files will be now be named usa-xx when they weretemplates-usa-xx before
  • Files inside /test-patterns directory caused error because it was building multiple files with the same name
    • Resolution Move test-patterns files inside appropriate component /test directories; update reference paths

Sass issues:

Issues discovered after running gulp buildSass

  • Error: Sass compilation errors found when pulling in packages/uswds via @import instead of @forward.
    • Using @import can cause compilation issues with nested elements inside placeholders, where elements are not included in the compiled file.
      • This Sass issue touches on the issue we discovered and recommended the switch from placeholders to mixins
      • Visual issues found in usa-table and usa-input
        image
        image
    • We need to support @import for backwards compatibility
    • Resolution - updated placeholders to mixins to reduce risk when @import is used
      • Note: the placeholders found in /placeholders/list and /placeholders/typography already had the needed mixins in the code base, so there was no need to add these to the /mixins directory

HTML build/Data issues:

Issues related to JSON files

  • The default json settings are merged into the components' variant data. This results in unintentional display issues for the following files inside /html-templates:
    • Sidenav - html template has two items with current class instead of one
    • Card - Flag variant has three items but only declares two
    • Footer - Five primary items are seen in footer--big but its json only declares four items
    • ⚠️ Note: this is not a bug but instead an intentional merge of data. In the future, we should return to this to make sure this is the correct strategic choice for how we structure our data
      • For now, we edited the default json to prevent merge issues in the variants
  • Card--flag: images display very tall
    image
    • Description data has content that is much longer than on current site
    • Resolution: Update data on card to improve visual presentation
  • Aria error in sidenav - Landmarks must be unique
    • Resolution: Add unique aria_label data to sidenav variants

Storybook issues:

Issues discovered in Storybook

  • Prose component is not included in storybook
    • Resolution: Added Prose component to storybook

@amyleadem amyleadem changed the base branch from library--develop to develop March 29, 2022 21:30
@amyleadem amyleadem changed the base branch from develop to library--develop March 29, 2022 21:30
@amyleadem amyleadem changed the title Beta - debug compare Beta - repair site integration issues Mar 30, 2022

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the files in test-patterns to their respective component directories?

Since these aren't used on the website, we can create a test section for each component story. Similar to "Components/Form Inputs/Checklist" for example.

Comment thread packages/usa-card/src/test/test-patterns/test-usa-card.json Outdated
@amyleadem amyleadem requested a review from mahoneycm April 11, 2022 17:42
@amyleadem

Copy link
Copy Markdown
Contributor Author

@mejiaj When you get a chance can you give this another look? I am still working on some final review for the USWDS-site sister PR but wanted to give you a chance to look at this if you had the time. I'll update you when USWDS-site is ready.

@mejiaj mejiaj self-requested a review April 11, 2022 19:45

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great so far, just a few minor comments and it's good to go.

Comment thread packages/usa-card/src/test/test-patterns/test-usa-card.json Outdated
Comment thread packages/usa-card/src/test/test-patterns/test-usa-card.json Outdated
Comment thread packages/usa-card/src/usa-card.stories.js Outdated
Comment thread packages/usa-checkbox/src/usa-checkbox.stories.js Outdated

.usa-input--success {
@include u-border($theme-input-state-border-width, "success");
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit confusing, but I think this particular partial is for the text input component in Fractal, here.

This used to be in %block-input-styles. I think we're okay because I see this package imported in the following components:

  • usa-hint
  • usa-icon-list ❓- Why does this component require input styles?
  • input prefix/suffix
  • memorable date
  • usa select
  • time picker
  • search
  • footer
  • fieldset
  • validation
  • form controls

@amyleadem amyleadem Apr 12, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mejiaj Do you know if any of these items need the usa-input--success class?

For reference, a little bit about my method:
To make the placeholder backwards compatible, I needed to:

  1. Remove the nested elements from the placeholder, which in this case was the usa-input--success class.
  2. Add the usa-input--success rules back in where needed
    1. To do this, I worked under the assumption that if there was an error class, there should also be a success. To find the error styles, I searched for error rules that have similar styling (in this case: @include u-border($theme-input-state-border-width) and then included a sibling success class.

If you can think of a better method or know of other components that need the usa-input--success styles, please let me know and I'll add them in.

Comment thread packages/usa-tooltip/src/test/test-patterns/test-usa-tooltip-utilities.twig Outdated
Comment thread packages/uswds-core/src/styles/mixins/general/table.scss Outdated
@mejiaj mejiaj requested a review from thisisdano April 12, 2022 19:23
@amyleadem amyleadem requested a review from mejiaj April 12, 2022 21:25

@mejiaj mejiaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

- Add default labels to `usa-password` component
- Cleanup password input id references, replace old references with a variable
- Pass data object explicitely to `usa-password`; might be a quirk with twigjs but empty object is passed if reference is used
Comment on lines +32 to +39
"toggle": {
"title": "Toggle password",
"aria_controls": password_id,
"text": {
"show": "Show password",
"hide": "Hide password"
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discovered this quirk while testing the sign-in-form component on Site.

What didn't work
Data isn't passed when building to HTML by reference in twig includes.

{% include "@components/_usa-password/src/usa-password.twig" with { toggle: password.toggle } %}

What worked
You can still use includes, but you have to write out the data you want to pass.

{% include "@components/_usa-password/src/usa-password.twig" with { 
  toggle: {
    …the data
  }
} %}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the digging into that @mejiaj. Much appreciated!

@thisisdano thisisdano merged commit eb10753 into library--develop Apr 15, 2022
@thisisdano thisisdano deleted the al-ld-remove-compare branch April 15, 2022 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants