Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade USWDS to 3.5.0 #37

Merged
merged 8 commits into from
Jul 6, 2023
Merged

Upgrade USWDS to 3.5.0 #37

merged 8 commits into from
Jul 6, 2023

Conversation

namanaman
Copy link
Contributor

  • Upgrades USWDS dependency from 3.0.1 to 3.5.0
  • Pulls in new components from USWDS
  • Updates README on how to release a new package version

@aws-amplify-us-east-1
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-37.d6umhtb6a6pvv.amplifyapp.com

Copy link

@casewalker casewalker left a comment

Choose a reason for hiding this comment

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

Hey hey. Not a lot to review here, looks good and is very exciting, but I left some comments about the Readme text and a lot of comments trying to standardize the ends of the USWDS files. However, maybe it is best that we just don't touch those files at all? Not sure, curious to know what you think.

Thanks!

LICENSE Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
</div>
</div>
</li>
</ul>

Choose a reason for hiding this comment

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

Do we control these files at all? Should we? If so, please add a newline at the end of this file.

Copy link
Contributor Author

@namanaman namanaman Jun 22, 2023

Choose a reason for hiding this comment

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

Reverting back to not changing these (see here for context #37 (comment)). But we pull most in automatically from USWDS

{% endfor %}
</tbody>
</table>
</div>

Choose a reason for hiding this comment

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

Please add a newline at the end of the file (if we control these files at all)

- name: zoom_out_map
meta: "expand fullscreen"
- name: zoom_out
meta: "shrink magnifying glass"

Choose a reason for hiding this comment

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

Please add a newline at the end of the file (if we control these files at all)

@@ -0,0 +1 @@
preview: "@uswds-form"

Choose a reason for hiding this comment

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

Please add a newline at the end of the file (if we control these files at all)

@@ -0,0 +1 @@
preview: "@uswds-form"

Choose a reason for hiding this comment

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

Please add a newline at the end of the file (if we control these files at all)

README.md Outdated Show resolved Hide resolved
@dhcole
Copy link
Member

dhcole commented Jun 22, 2023

@namanaman I notice two things here: https://pr-37.d6umhtb6a6pvv.amplifyapp.com/components/preview/header--extended.html

  • The search icon appears broken
  • The contrast between the utlity links and the white background seems too low

Not sure if these are upstream issues or something we introduced

@dhcole
Copy link
Member

dhcole commented Jun 22, 2023

  • The search icon appears broken

Noting this does not appear to be the case on the basic header: https://pr-37.d6umhtb6a6pvv.amplifyapp.com/components/detail/header--basic.html

@dhcole
Copy link
Member

dhcole commented Jun 22, 2023

The multiselect accordian doesn't appear to be multiselect: https://pr-37.d6umhtb6a6pvv.amplifyapp.com/components/preview/accordion--multiselectable.html

@dhcole dhcole self-requested a review June 22, 2023 00:49
Copy link
Member

@dhcole dhcole left a comment

Choose a reason for hiding this comment

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

Just noticed... the USWDS repo changed its file structure, so we actually have a bunch of duplicate components now. See here: https://github.com/newjersey/njwds/tree/upgrade-uswds/src/components

Before, they had components in directories prefixed with numbers for sorting, but in a more recent change they removed these numbers, so many components appear with both formats.

Also noticing the most recent main branch in the USWDS repo looks like it's switched to storybook and done away with these components altogether...so I'm not actually sure where these new components are coming from, unless you have them locally from a previous (but more recent) uwds version. https://github.com/uswds/uswds/tree/v3.5.0/src

@dhcole
Copy link
Member

dhcole commented Jun 22, 2023

Some discussion here... uswds/uswds#3263 but not finding any clear communications on the rationale or migration strategy... I'd say we just clean up the duplication for now and then revisit what we want to be our upstream system as a more comprehensive task.

@namanaman
Copy link
Contributor Author

@dhcole Thanks for looking into this further, I should have done some more careful inspection of these changes after importing... So it looks like npm run import-components is copying files from the node_modules/uswds directory. I realized we have a node_modules/@uswds/uswds directory, with the latest 3.5.0 NPM package, AND a node_modules/uswds directory with version 2.2.1, which is pulled as part of a uswds-gulp dependency. As you mentioned, the components I pulled in don't exist in USWDS 3.5.0, and have been replaced by Storybook stories in 3.0.0 and after. Based on that, I'm thinking the following:

  • In this PR, I will upgrade to USWDS 3.5.0 but not import any new NJK components, to keep the behavior as it was. Anyway, these components are coming from an old package, so not much point in pulling them in.
  • Consider migrating to uswds-compile #33 Use the new recommended uswds-compile instead of uswds-gulp. This will take away the confusing dependency of that extra older package in our system. This is useful anyway as uswds-gulp is being deprecated.
  • Migrate from Fractal to Storybook component library #38 Migrate Fractal to Storybook for NJWDS so we can easily import the USWDS stories upstream

Copy link
Member

@dhcole dhcole left a comment

Choose a reason for hiding this comment

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

Looks great!

package.json Show resolved Hide resolved
@namanaman namanaman merged commit 412752e into main Jul 6, 2023
@namanaman namanaman deleted the upgrade-uswds branch July 6, 2023 21:00
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