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

Add support for localisation via JavaScript configuration to Accordion component #2826

Merged
merged 5 commits into from
Sep 12, 2022

Conversation

querkmachine
Copy link
Member

@querkmachine querkmachine commented Sep 2, 2022

This is related to two issues, #2802 and #2698, as I couldn't find a way to cleanly separate them.

  • Modifies the initAll function to pass options.accordion into the accordion initialisation.
  • Modifies the accordion component to accept a second config parameter (defaults to an empty object).
  • Modifies the accordion component's config merging to accept the config parameter.
  • Updates the 'translated' full-page example to call initAll with a custom configuration.
  • Adds integration tests checking that the accordion's text is modified according to the configuration.
  • Adds unit tests to the existing mergeConfig function to ensure that it supports empty objects.

Todo

#2802 includes a task to ensure that "Data attribute translations take priority over JS translations, if both provided"

This is partially covered by the configuration merging function's unit tests, though that function also doesn't make an explicit distinction between configuration passed from data-* attributes and those from JavaScript. Do we feel like we need to explicitly test for parameter ordering?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-2826 September 2, 2022 16:14 Inactive
Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

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

LGTM! 👍🏻

src/govuk/components/accordion/accordion.test.js Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants