Fixed deprecation warnings and bugs on page operations #11
Conversation
|
@cabidop Thanks! I'll take a closer look as soon as I can. |
|
Hi, @angusmcleod ! Any chance to take a look? |
There was a problem hiding this comment.
@cabidop Thanks a lot for this! I really appreciate the effort you've put in here. I've left a few specific comments.
On the whole, before I can support this is a merge that bring the plugin up to date with Discourse, There are a few changes here that seem to not be strictly about making the plugin work, and it's not clear to me whether they're breaking some existing non-inline functionality.
I definitely don't want to discourage you! But there may need to be some more focus on the other functionality you haven't been directly using, before I can support this as a general "fix". Essentially, when we make that claim by merging this I don't want to be signing up for a fresh round of support from folks who want to use other features that are newly broken, or still broken.
But keen to support you here still. If you'd like more specific feedback than I've given, let me know.
| @@ -2,7 +2,7 @@ | |||
| class LandingPages::InvalidAccess < StandardError; end | |||
| class LandingPages::InvalidParameters < StandardError; end | |||
|
|
|||
| class LandingPages::LandingController < ::ActionController::Base | |||
| class LandingPages::LandingController < ApplicationController | |||
There was a problem hiding this comment.
@cabidop Inheriting the application controller here means inheriting quite a lot of logic. Could you walk me through why you're doing this? Perhaps this is to make the new "inline" pages you've added? Does it work for non-inline pages though?
There was a problem hiding this comment.
Not directly related to the inline pages (although it simplified quite a bit the implementation), but checking the ActionController::Base documentation seems that by default the only controller that inherits it is the ApplicationController, which seems to also be the approach for the default and official Discourse plugins I checked out (e.g. chat, poll, styleguide, calendar, cakeday, etc).
On the other hand, I have not seen anything specifically discouraging to inherit ActionController::Base directly so let me know your thoughts on this.
There was a problem hiding this comment.
Yes, typically you would inherit the ApplicationController because you're serving content within Discourse and you want all the logic included in the ApplicationController (e.g. all it's before_actions). However in this case, sometimes we don't want all of that logic running because a landing page will not be running inside of the main Discourse app. If there wasn't a good reason to make this change then best not to make it.
app/views/layouts/landing.html.erb
Outdated
| @@ -24,7 +24,8 @@ | |||
| <%= discourse_stylesheet_link_tag "landing_page", theme_id: nil %> | |||
|
|
|||
| <%- if theme_id.present? %> | |||
| <%= discourse_stylesheet_link_tag (mobile_view? ? :mobile_theme : :desktop_theme), theme_id: theme_id %> | |||
| <%= discourse_color_scheme_stylesheets %> | |||
| <%= discourse_stylesheet_link_tag (mobile_view? ? :mobile : :desktop), theme_id: theme_id %> | |||
There was a problem hiding this comment.
@cabidop Whether it's this change or some other changes, page styling of non-inline pages seems to be broken. For example if you import pages from https://github.com/paviliondev/pavilion-landing-pages and apply the theme https://github.com/paviliondev/pavilion-landing-theme you see this
There was a problem hiding this comment.
Absolutely right, seems that both tags are required as the :mobile/:desktop is used for the base styles and the :mobile_theme/:desktop_theme is used for the imported themes and the "manual" updates on the default one.
There was a problem hiding this comment.
Also the current code it is actually ignoring the page setting for the theme, as all the conditionals use theme_id.* instead of @page.theme_id.*.
|
@cabidop Thanks for the followup. Let me know when you've made those changes. I've added standard plugin CI in here as a sanity check. That will just run the rspec suite in the plugin. We'll need to make sure that's passing before we merge this. Thanks again! |
cabidop
changed the title
|
Hey @angusmcleod, thanks a lot for the review! I have just pushed some additional commits to address your comments. I fully agree on leaving out the new "inline" functionality (I will add it as a standalone change once the basics are fixed), and I rephrased the PR itself to better reflect its intention. It is now clear that it only impacts the page related functionality, leaving the fixes for both the global and remote functionalities for a different PR. I also tested the spec locally and added two small fixes for it, but I am unsure if the CI pipeline will run before merging the PR (as the file is obviously not yet in the |
|
@cabidop Ok! I think we can address further issues with the plugin separately. You've achieved a baseline of functionality here that we should bank. Thanks again! In terms of follow up:
If you're keen to use this for the long haul I can also make you a maintainer. Let's see how we go in that respect. |
Hi, @angusmcleod! Not sure if you are paying any attention at all to the plugin, but I recently started working with Discourse and found that Landing Pages filled exactly the gap I just needed for a project. Seeing that it was unmaintained I started my own simplified version of your plugin, but this week I decided to give it a go and update yours so here we are now 馃槃 .
Although I am already using my fork, I would appreciate if you could take a look to this PR to include the changes in the main repository so everyone can benefit from them. The list is not huge as I mainly focused on removing deprecation warnings and bringing the basic functionality back to life:
There are still some small issues with the save functionality (e.g. you can never remove categories and group once you added them) and I have not reviewed yet the global and remote functionality, but at least the plugin is now on a non-broken state.
If you think it is too much to review in one go let me know as I can definitely make multiple PRs, each commit is more or less self-contained. Thanks a lot in advance and hope that you are still around for this!