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 checking what components use govuk-frontend to components audit #3716

Closed

Conversation

patrickpatrickpatrick
Copy link
Contributor

@patrickpatrickpatrick patrickpatrickpatrick commented Nov 14, 2023

What

Adds checking to see what components use govuk-frontend to the components audit.

Why

Implemented this to double check that I wasn't missing out any component JS when making changes to applications to support govuk-frontend v5. Thought it could be useful if this was a standard feature in the auditing, so I have opened this PR.

Visual Changes

Before

Screenshot 2023-11-14 at 14 50 07

After

Screenshot 2023-11-14 at 14 49 55

@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3716 November 14, 2023 10:03 Inactive
@patrickpatrickpatrick patrickpatrickpatrick changed the title Add checking what components use govuk-frontend Add checking what components use govuk-frontend to components audit Nov 14, 2023
@andysellick
Copy link
Contributor

Someone else is adding stuff to the auditing tools 🎉

Bit confused as to the purpose behind this - why do we need to know which applications use components that use govuk-frontend JS? I thought if we included a component JS in an application that included any govuk-frontend JS as well, or is that going to change?

@patrickpatrickpatrick
Copy link
Contributor Author

patrickpatrickpatrick commented Nov 14, 2023

Someone else is adding stuff to the auditing tools 🎉

Bit confused as to the purpose behind this - why do we need to know which applications use components that use govuk-frontend JS? I thought if we included a component JS in an application that included any govuk-frontend JS as well, or is that going to change?

In the approach I'm doing to ensure govuk-frontend isn't evaluated/run by browsers that don't support it (browsers that don't support type="module"), I'm moving the components that use the ES6 JS from govuk-frontend to a different file. This file will then be imported using a type="module" tag so that if browser doesn't support the tag then they won't run the modules.

This information just lets me know what components that are using ES6 need to be moved to a separate file and where they are so I can test them. If we are dividing the upgrading of applications between the teams that are responsible for maintaining them, then I thought others might find this information useful.

@andysellick
Copy link
Contributor

In that case can I suggest an alternative approach? Rather than add a whole new section, could you modify the existing 'gem components used by applications' to somehow highlight those components using govuk-frontend? Something like this maybe?

Screenshot 2023-11-14 at 11 58 47

It also might be useful to have this information included in the main 'component files' section, although there's limited width there already so might not be practical.

@patrickpatrickpatrick
Copy link
Contributor Author

In that case can I suggest an alternative approach? Rather than add a whole new section, could you modify the existing 'gem components used by applications' to somehow highlight those components using govuk-frontend? Something like this maybe?

Screenshot 2023-11-14 at 11 58 47

It also might be useful to have this information included in the main 'component files' section, although there's limited width there already so might not be practical.

Ah yep, that makes more sense actually! I'll make that change

@patrickpatrickpatrick patrickpatrickpatrick force-pushed the audit-for-govuk-frontend-components branch from 298c940 to 4138e1a Compare November 14, 2023 14:49
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3716 November 14, 2023 14:50 Inactive
@patrickpatrickpatrick patrickpatrickpatrick force-pushed the audit-for-govuk-frontend-components branch from 4138e1a to c3fcb31 Compare November 15, 2023 10:32
@patrickpatrickpatrick patrickpatrickpatrick changed the base branch from try-to-update-govukfrontend to main November 15, 2023 10:33
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3716 November 15, 2023 10:33 Inactive
@patrickpatrickpatrick patrickpatrickpatrick force-pushed the audit-for-govuk-frontend-components branch from c3fcb31 to 94db499 Compare November 15, 2023 10:34
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3716 November 15, 2023 10:34 Inactive
@patrickpatrickpatrick patrickpatrickpatrick force-pushed the audit-for-govuk-frontend-components branch from 94db499 to c4253cc Compare November 15, 2023 10:35
@govuk-ci govuk-ci temporarily deployed to components-gem-pr-3716 November 15, 2023 10:36 Inactive
@andysellick
Copy link
Contributor

@patrickpatrickpatrick would you like a review of this PR?

@patrickpatrickpatrick
Copy link
Contributor Author

Ah yes please @andysellick ! Think this will be useful for anyone upgrading to v5 of govuk-frontend.

Copy link
Contributor

@andysellick andysellick left a comment

Choose a reason for hiding this comment

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

Code changes look good, would be good to get a test included for this if possible. There's a load of test files in /spec/dummy_gem/ that could be adapted, see the other auditing tests.

If a component being used in an application is uses govuk-frontend
ES6 JS, then a tag will be added to indicate this in the Component
Audit view. This is useful to determine which components are using
ES6 JS and need to have their JS be loaded in a different file.
@andysellick
Copy link
Contributor

@patrickpatrickpatrick am closing this but have used it as the basis for #4058, thanks for suggesting 👍

@andysellick andysellick deleted the audit-for-govuk-frontend-components branch June 12, 2024 12:47
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