Skip to content

Improve accuracy of Frontend packages, events documentation#9814

Merged
aduth merged 3 commits intomainfrom
aduth-frontend-docs-accuracy-pkg-events-mappping
Dec 21, 2023
Merged

Improve accuracy of Frontend packages, events documentation#9814
aduth merged 3 commits intomainfrom
aduth-frontend-docs-accuracy-pkg-events-mappping

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Dec 20, 2023

🛠 Summary of changes

Updates the Frontend Architecture documentation to improve a few inaccuracies:

  • We don't require listing sibling package dependencies between Yarn workspace packages
  • private must be specified in all package.json (enforced by scripts/validate-workspaces.js), not just for published packages
  • Generalize the recommendation around what was intended with the index.js documentation, also using the recommended TypeScript file extension, and allowing equivalent exports or main
  • Describe JavaScript package integration using ViewComponents, which is generally preferable to packs
  • Describe ALLOWED_EVENTS accurately as an allowlist, rather than a mapping

📜 Testing Plan

Verify that documentation revisions are sensible.

changelog: Internal, Documentation, Improve accuracy of frontend architecture documentation
This mapping associates the event name logged from the frontend with the corresponding method from
[AnalyticsEvents][analytics_events.rb] to be called. All properties will be passed automatically to
the event from the frontend as long as they are defined in the method argument signature.
This is an allowlist of events defined in [AnalyticsEvents][analytics_events.rb] which are allowed
Copy link
Contributor

Choose a reason for hiding this comment

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

does this URL need to point at the actual file?

Suggested change
This is an allowlist of events defined in [AnalyticsEvents][analytics_events.rb] which are allowed
This is an allowlist of events defined in [AnalyticsEvents][../app/services/analytics_events.rb] which are allowed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment is making me realize that the way I went about this is prone to causing confusion, but these are using reference links, defined at the end of the heading section:

[frontend_log_controller.rb]: https://github.com/18F/identity-idp/blob/main/app/controllers/frontend_log_controller.rb
[analytics_events.rb]: https://github.com/18F/identity-idp/blob/main/app/services/analytics_events.rb

@@ -138,9 +136,9 @@ See [`@18f/identity-analytics` package documentation][analytics_package] for cod
how to track an event in JavaScript.

Any event logged from the frontend must be added to the `ALLOWED_EVENTS` allowlist in [`FrontendLogController`][frontend_log_controller.rb].
Copy link
Contributor

Choose a reason for hiding this comment

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

same: fix path?

Suggested change
Any event logged from the frontend must be added to the `ALLOWED_EVENTS` allowlist in [`FrontendLogController`][frontend_log_controller.rb].
Any event logged from the frontend must be added to the `ALLOWED_EVENTS` allowlist in [`FrontendLogController`][../app/controllers/frontend_log_controller.rb].

@aduth aduth merged commit 0297e22 into main Dec 21, 2023
@aduth aduth deleted the aduth-frontend-docs-accuracy-pkg-events-mappping branch December 21, 2023 13:24
@jmdembe jmdembe mentioned this pull request Dec 21, 2023
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