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

Feature/add hods theme #412

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

andrewgibson-ibm
Copy link
Collaborator

Description

Current HOF framework enables the use of govUK theme which implements the components and styling designed in the Government Design System. This update enables the use of the Home Office Design System by implementing the relevant styles and page template.

Changes

  1. All styling changes are described on the HODS website
  2. The TaskList component has been added
  3. Document title has been changed to {{pageTitle}} - {{appTitle}}
  4. Build task now includes copying over font assets if they exist

Testing

  1. Import this fork as hof package in your app
  2. In hof.settings.json, set "theme": "hods",
  3. Run npm i, then npm run start to start your app
  4. Observe that the changes listed above are implemented

@@ -15,4 +16,4 @@ coverage
.nyc_output
build/Release
node_modules
.emails
.emails
Copy link
Contributor

Choose a reason for hiding this comment

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

new line? you can add this in your editor to prevent this happening in the future

if (e.code !== 'ENOENT') {
throw e;
} else {
console.log(`${chalk.yellow('warning')}: no fonts directory found at ${config.fonts.src}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice use of chalk 👍

.then(() => spawn('cp', ['-r', config.fonts.src, config.fonts.out]))
.catch(e => {
if (e.code !== 'ENOENT') {
throw e;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand this, if you catch an error that doesn't have a specific path/ file directory, you'll throw an error? Would it be better to provide a helpful error message?

@@ -0,0 +1,14 @@
{
"name": "hmpo-govuk-template-example",
"description": "Example app for hmpo-govuk-template module",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it say hmpo?

</div>
{{/main}}

{{/ hods-template}}
Copy link
Contributor

Choose a reason for hiding this comment

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

new line

/* eslint-disable max-len, no-var, vars-on-top, no-undef */
'use strict';

// TODO: update package.json(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

what do you need to update in package.json(s) would it be best to do as this PR?

});
}
}
}).call(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing this is just pull it out from hods? if it is, then that's fine if we test it and it works


.hmpo {
white-space: nowrap;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new line

@sulthan-ahmed
Copy link
Contributor

I see that the styles are being copied across rather than having an npm module. I had a look at the HODs site and it looks like that's the way to do it. I sent a message to the team

Hi what is the approach to import the ho-design system for production?
@Andrew Gibson
has written some code for our HOF framework to use the design system. I’m just reviewing it.
Do you have an npm module for the ho-design system something similar to govuk-frontend, it would make it easier to import the styles?
I wonder if copying and pasting the styles would be difficult to maintain when the design system is updated? CC
@daniel
(edited)


.hmpo {
white-space: nowrap;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a shame you have to copy the sass and there is no npm module for the sass

.govuk-back-link:link,
.govuk-back-link:visited {
color: #005ea5 !important;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

newline

}

}

Copy link
Contributor

Choose a reason for hiding this comment

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

are all the empty lines on purpose?

@serviceops
Copy link

serviceops commented Jan 30, 2024 via email

@sulthan-ahmed
Copy link
Contributor

This is really great work @andrewgibson-ibm there's just some minor linting things to sort out. I also have a question about the sass. Am I right in thinking it has been copied and pasted from the HO design system? If that's the case, then this will make it difficult to maintain in the future. For example if the HO design system updates and changes the sass how can we tell what has changed and how will we update it in the future. I don't know the answer to this so it would be good to discuss.

Once we can come to a way forward with this, the next bit would be to publish a beta package of this and test it on multiple services.

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