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

Fixing error message issue #618

Merged
merged 4 commits into from
Aug 8, 2024
Merged

Fixing error message issue #618

merged 4 commits into from
Aug 8, 2024

Conversation

nnunn
Copy link
Contributor

@nnunn nnunn commented Aug 8, 2024

Error messages were not working because this application file had references to cms files. I have removed these. Also removed the dark mode and language selectors as we don't need these in the application layout.

In addition, I have changed the mark-up for the error pages so they display nicer.

nnunn added 4 commits August 8, 2024 14:45
Error messages were not working because this application file had references to cms files. I have removed these. Also removed the dark mode and language selectors as we don't need these in the application layout.
formatting changes.
@nnunn nnunn requested review from pgwillia and murny August 8, 2024 20:53
<% else %>
Library
<% end %>
U of A Library
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why you removed the content for(:title)/cms_page.label stuff? This seemed useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can tell the cms calls were creating the error so I decided to just remove them.

<!-- Google Tag Manager -->
<script>(function(w,d,s,l,i){w[l]=w[l]||[];w[l].push({'gtm.start':
new Date().getTime(),event:'gtm.js'});var f=d.getElementsByTagName(s)[0],
j=d.createElement(s),dl=l!='dataLayer'?'&l='+l:'';j.async=true;j.src=
'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) %>
Copy link
Collaborator

@murny murny Aug 8, 2024

Choose a reason for hiding this comment

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

You could just wrap this in a check? If we have a @cms_site then we are in the CMS, so we want the stylesheets/js from the CMS. Otherwise we are not in the CMS, so don't include the css/js from the CMS? So it works for both worlds?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could do - but as far as I can tell the only thing that uses the application layout are these error pages. This seemed like the quickest and easiest way to get everything working :)

@nnunn
Copy link
Contributor Author

nnunn commented Aug 8, 2024

Thanks for approving @murny let me know if you want to make the changes you suggested above. Otherwise, I'm hoping we can cut a release next week when Neil is back from folk fest.

@murny
Copy link
Collaborator

murny commented Aug 8, 2024

Nothing blocking on my side. So all is good 👍. Just some suggestions to try to have this layout work in both CMS/non CMS world (but probably everything in CMS world is using the basic layout one)

@nnunn nnunn merged commit 3ebe49d into master Aug 8, 2024
2 checks passed
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.

2 participants