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

Review home-office-kit plugin #2190

Open
4 tasks
ruthhammond opened this issue May 25, 2023 · 3 comments
Open
4 tasks

Review home-office-kit plugin #2190

ruthhammond opened this issue May 25, 2023 · 3 comments
Labels
Development Next task sits with developer discipline 🕔 Hours A well understood issue which we expect to take less than a day to resolve.

Comments

@ruthhammond
Copy link

ruthhammond commented May 25, 2023

What

Review the home-office-kit plugin and provide feedback, including things that we think need to be changed.

Linked to PR #2135

Why

So the plugin authors know what they need to do for the plugin to be listed in the 'manage your prototype' section

Who needs to work on this

Developer

Who needs to review this

Team

Done when

  • A review is completed
  • A list of recommendations is drafted and shared with the GOV.UK Prototype Kit team
  • The recommendations are fed back to the author
  • We use this review to create a template for future plugin reviews
@ruthhammond ruthhammond added the 🕔 Hours A well understood issue which we expect to take less than a day to resolve. label May 25, 2023
@ruthhammond ruthhammond added Development Next task sits with developer discipline Priority 1 Tickets lined up for the upcoming sprint labels Jun 2, 2023
@ruthhammond ruthhammond removed the Priority 1 Tickets lined up for the upcoming sprint label Jun 14, 2023
@HannahJMWood
Copy link
Contributor

Review results:

  • ran the plugin validator to check the config, no issues were found with it

  • namespacing does not seem to be consistent

    • css is namespaced “hods"
    • nunjucks macros are name spaced "home-office-kit”
    • nunjucks paths are not namespaced (eg. they are just "/layouts" and "/macros”)

@HannahJMWood HannahJMWood self-assigned this Jun 28, 2023
@BenSurgisonGDS
Copy link
Contributor

BenSurgisonGDS commented Jun 28, 2023

I don't know if we have this as a requirement, but I think the script within the script tag in their templates should be a callback wrapped within a call to window.GOVUKPrototypeKit.documentReady()

eg:

<script>
window.GOVUKPrototypeKit.documentReady(() => {
    /* their script code goes here */
 })
</script>

This is to make sure the kit is ready before the script is executed.

@joelanman
Copy link
Contributor

Is this just about namespacing in which case we can meet and agree on a preliminary rule for that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development Next task sits with developer discipline 🕔 Hours A well understood issue which we expect to take less than a day to resolve.
Projects
None yet
Development

No branches or pull requests

4 participants