search

alphagov / govuk-prototype-kit

Rapidly create HTML prototypes of GOV.UK services
https://prototype-kit.service.gov.uk
MIT License
306 stars 236 forks source link

Investigate simplifying the update process - application.scss #1377

Closed joelanman closed 2 years ago

joelanman commented 2 years ago

What

There are 4 steps to update application.scss (steps 4 - 7): https://govuk-prototype-kit.herokuapp.com/docs/updating-the-kit

We could replace the relevant lines in application.scss with a single import for all of 'our stuff', so in future users don't have to do this manually.

Why

This significantly simplifies the update process, making it quicker and more reliable

Who needs to work on this

Who needs to review this

Done when

lfdebrux commented 2 years ago

This is a really good idea, I think we could go even further and remove the need for the user's application.scss to have any imports in it if we invert the control, and have a lib/kit.scss that imports app/assets/scss/application.scss.

Do you think this would be a breaking change? I'm wondering if we could put this out in a minor release.

joelanman commented 2 years ago

If there was an update guide, I don't think this would be a breaking change. I think we'd need to consider adding something to the update script, otherwise we'd have to include these steps for anyone who hasn't updated to the new method. And we'd probably have to complicate the steps even further ("If you're on version X...")

lfdebrux commented 2 years ago

Hmm, good point about updating. I'm not sure how we could make the update script do it for the user, but if we didn't find a way then it could end up making the update process more confusing... So we should definitely try and find a way. I'll have a think about it.

joelanman commented 2 years ago

If we used your import idea, the script would just have to delete everything above // Add extra styles here

lfdebrux commented 2 years ago

If we used your import idea, the script would just have to delete everything above // Add extra styles here

Yeah that could work, or deleting the lines that we used to expect... I think it need careful testing though, as users can always do unexpected things...

joelanman commented 2 years ago

agree, though this has been the 'api' for a long time, so I think it would be ok to break it and support users who put their own code above that line to fix it

lfdebrux commented 2 years ago

@BenSurgisonGDS has investigated this in https://github.com/alphagov/govuk-prototype-kit/pull/1385.

He found that instead of just moving application.scss to the lib folder, it made the most sense to move the whole app/assets/sass folder, firstly because it makes the most sense when you try and understand the different Sass imports involved, and also because it greatly reduces the amount of code in the app folder that users haven't made.

However this approach does raise some interesting challenges and questions:

joelanman commented 2 years ago

I also discussed this a bit with Ben

  1. I would say no to the ie8 files, in fact we should consider dropping them. Unbranded I think needs a bit of investigation. If people have done their edits in application.scss (which unbranded imports), or they just haven't made any edits, then maybe we don't need to have an unbranded in user space

  2. Again worth investigating what people have done, but I think importing and using sass might be an advanced technique that our users don't use

  3. I think we should leave patterns folder in the sass folder, move our patterns to lib as they belong to us, and not touch any files that we didn't make, I'm aware of users who have added their own patterns to that folder.