Code Cleanup and new Google Tag Manager #607
Conversation
|
Deleting unused layouts seem to be breaking our tests... |
app/views/layouts/mailer.html.erb
Outdated
There was a problem hiding this comment.
These are just default Rails mailer layouts that came out of the box, they probably fine to leave?
Maybe one day we might want to send emails or something 🤔
There was a problem hiding this comment.
We've never sent emails so I thought it would be cleaner not to have these at all...
There was a problem hiding this comment.
If you remove them, then I'd remove the mailer class which uses them?
https://github.com/ualbertalib/library-cms/blob/master/app/mailers/application_mailer.rb
Otherwise, I would just leave them alone
There was a problem hiding this comment.
Ok great - I reverted those deletions!
| 'https://www.googletagmanager.com/gtm.js?id='+i+dl;f.parentNode.insertBefore(j,f); | ||
| })(window,document,'script','dataLayer','GTM-MX43PRW2');</script> | ||
| <!-- End Google Tag Manager --> | ||
| <%= stylesheet_link_tag comfy_cms_render_css_path(@cms_site.id, @cms_layout.identifier) %> |
There was a problem hiding this comment.
Profile tests that I added caught an error here. When staff/profile pages get rendered, we don't have a @cms_site.id available, and as a result it's throwing a Nil no method error here?
Might have to handle this smarter for places where we are using application.html.erb outside the CMS 🤔
There was a problem hiding this comment.
I don't think we'll be using application.html.erb outside of the CMS anymore. The staff/profile pages are all being replaced with a springshare product tomorrow...
There was a problem hiding this comment.
Should we create a new PR to remove all this code then? Once this is merged, then this PR is good to go?
There was a problem hiding this comment.
Yeah - I was thinking I would do another code release next week once we have our replacement staff directory in place.
There was a problem hiding this comment.
So yes, once this is merged - I'll get Neil to deploy the new release tomorrow. Is there anything else you'd like to add?
|
@murny does this look ok to you now? I'm hoping to cut a release -- if not we can wait till Neil is back next week. |
@nnunn the CI pipeline is red right now, so even if I approve you cannot merge this. We need to do something about profiles code and the application layout. If you want I can create a pull request to remove all the profiles stuff and then we can merge this. Once this is merge then we can merge this pull request. |
That would be much appreciated! |
Removing chat widget - this can go in the main layout within the cms
Updating application layout (which creates the error pages for the application)
Layout no longer used
Layout no longer used
Layout no longer used
Layout no longer used.
Layout no longer used
Layout no longer used
Layout no longer used.
Layout no longer used
This reverts commit efa975a.
This reverts commit aa10f09.
|
I rebased this pull request with the latest master (which has the profile removal code), so CI pipeline should be green now. Will approve this now 👍 |
|
Tests are now failing on missing layout of I see we using the following layouts in the CMS seed data:
Any ideas here @nnunn? Should these just be replaced with |
|
I think we should probably remove the old seed data and recreate it with the current pages. This is a pretty simple rake task comfy:cms_seeds:import & comfy:cms_seeds:export but it needs to be run directly in prod to get the latest data. For now we could change all those layouts to "basic" |
|
Cool, didn't know about the import/export rake task. When I talk to Neil about doing a release next week, we can ask him to get a new export dump of the seed data in prod. For now, I can make the changes to update them to use "basic" layout 👍 |
There was a problem hiding this comment.
I updated all the layouts in the cms seed data to use basic layout.
Unfortunately, there is a ton of spacing diffs here (trailing whitespace that my editor trims, etc). But the only real change is updating app_layout to app_layout: basic
|
Everything seem to be okay locally, but some CSS is a bit off. Think this is just due to old CMS data. We need a new CMS data dump from prod, so I added a backlog issue for this: #610 Think this is okay to merge for now, will allow to us to continue to clean up the other unused code that we have. |
Code Cleanup and new Google Tag Manager code from central. Deleted a number of unused layouts. Also removed the chat widget from the basic layout as this should go in the English and French main layouts in the CMS. Also added navigation to the application layout so that error pages have the new look and feel.