Skip to content

Document components JavaScript package and exclude feature-specific components#6756

Merged
aduth merged 2 commits intomainfrom
aduth-components-package
Aug 16, 2022
Merged

Document components JavaScript package and exclude feature-specific components#6756
aduth merged 2 commits intomainfrom
aduth-components-package

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Aug 16, 2022

Context: #6624 (comment)

Why:

  • Since components are meant to be generalized and reusable, not tied to a particular feature (particularly if a package already exists for said feature).
  • To improve clarity on what the package is expected to contain.

aduth added 2 commits August 16, 2022 10:32
**Why**: Since components are meant to be generalized and reusable, not tied to a particular feature (particularly if a package already exists for said feature).
**Why**: To improve clarity on what the package is expected to contain.

changelog: Internal, Documentation, Add documentation for JavaScript packages

<p>{t('in_person_proofing.body.location.location_step_about')}</p>
<LocationCollection>{locationItems}</LocationCollection>
{locationsContent}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change here fixes issue where previously we were rendering invalid content inside the LocationCollection's <ul>:

Permitted content | Zero or more <li><script> and <template> elements.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ul

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

reusable, whereas domain-specific React components should be found in the package corresponding to
the specific page or feature.

Many of these components are React implementations for components of the [Login.gov Design System](https://design.login.gov/).
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a rule we could enforce/lint on if we wanted, maybe. Like each React component in this dir should have a comment/metadata attribute of some sort that links to its corresponding LGDS component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not a hard rule, since we have some components which don't map directly to a design system component (e.g. PageHeader). Conversely, we have some design system components which live elsewhere (e.g. step-indicator).

In an ideal world, I'd like to move more toward per-component packages (also aligns with newer USWDS direction). But I also think that might be a bit too much overhead and may be on the more extreme side of granularity, impacting usability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In an ideal world, I'd like to move more toward per-component packages (also aligns with newer USWDS direction). But I also think that might be a bit too much overhead and may be on the more extreme side of granularity, impacting usability.

On the other hand, the overhead could be a good thing to require more conscious effort to introducing these generalized components, since I think this current components package is too much at risk of including any and all sorts of React components. Plus, it would resolve the current inconsistency of some components living in this package, while others have their own package.

@aduth aduth merged commit d797567 into main Aug 16, 2022
@aduth aduth deleted the aduth-components-package branch August 16, 2022 18:09
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.

2 participants