Better View Customisation

[shettytejas]

Why

It's been a while since I've started using Phlex. While checking this gem out, I figured that Phlex wasn't working correctly, and doing some changes makes it work nicely while keeping the original behaviour.

What's Changed

1. ~Added a new #render accessor to SudoRails. Valid values can be of types String and a Proc.~
2. ~Added a method #get_render. Used this method in sudo class method. By default, it'll always return 'sudo_rails/confirm_form'.~
3. ActiveSupport.on_load hook is registered for :action_controller_base instead of :action_controller (Inclusion SudoRails::ControllerExt was happening on ActionController::Api in some cases).
4. Added a name to the confirm route to use it easily using a route helper.
5. Added a generator for confirm_form.html.erb

[markets]

Looking good @shettytejas! Do you think we can add a test? I also added some comments to be checked 🙏🏼

[shettytejas]

Looking good @shettytejas! Do you think we can add a test? I also added some comments to be checked 🙏🏼

I've added some test cases along with one extra case with a Phlex component itself which replicates my real world usecase.

[markets]

Hey @shettytejas 👋🏼 sorry but I'm a bit confused about the render argument and the whole direction here.

I wouldn't like to stop people using Phlex, ViewComponents, ... to use this gem, but not sure if this is the path we should take.

Let me explain a bit: ok, now you're able to render a different view, but if you take a look to this view, you will see that there some logic and settings. If someone render a custom Phlex view, they will need to re-implement the same logic and options? hmmm 🤔 not sure if this is proper way to do that. On the other hand, if someone is using Phlex, since ERB is default Rails renderer, what's the problem to use ERB for internal views from gems. Does, for example, Devise (or any other gem relevant with views) provide a similar solution? I'm almost sure they not.

[shettytejas]

So imo, it's not just about rendering a different view, but also a way to entirely customise the view based on the design choices. For example, in my use case, I'll be using sudo_rails on a user facing application, and the included style(s) is totally different from the other parts of the app even with the color overrides (Let's say for example I use a UI component library like Daisy UI which is very customised with tailwind classes).

While I built this PR with Phlex being supported as my primary implementation goal, the bigger requirement is a way to make the view align to the application's design choices.

I've never used Devise tbh but from what I've seen in many libraries, I would be able to generate the same view in the local codebase and override the views according to my taste (one of the libraries I'm talking about is madmin)

We can build a generator for the same which is a good way (and if it's already available, I'm sorry to miss it), but in my honest opinion, if somebody want's to keep things consistent by using a different component library, I think there should be a way to provide it. This does come with the reimplementation of the password logic, which is a good point, but again if someone is thinking of changing the rendering view, they should be doing that on their own risk. A good way to mitigate it is adding some documentation to let the developers know they can do change things their way, but they should keep these things in mind or something.

What are your thoughts?

[markets]

I agree with the fact that styling should be customizable. I'm also using this gem in a couple of projects and I matched the styles of the app by using the same layout as my main app (instead of the default sudo_rails/application) and then using some css overrides (at the end, they are just a couple of selectors: .sudo-header, .sudo-form). That worked great for me and my team.

On the other hand, you can already override (as on any Rails Engine) the default views, by just placing a file in your app at app/views/sudo_rails/confirm_form.html.erb, where you can even call something like <%= render MyComponent.new %>. So maybe we can just properly document somewhere this 🤔?

I'm ok with these 2 changes:

3. ActiveSupport.on_load hook is registered for :action_controller_base instead of :action_controller (Inclusion SudoRails::ControllerExt was happening on ActionController::Api in some cases).
4. Added a name to the confirm route to use it easily using a route helper.

[shettytejas]

That make sense and is totally doable! However, my concern with that approach is it's more verbose and patchy, in the sense that I need to explicitly use a template to refer to another template, while providing a way to have it implicitly render would be the same functionality, and more pleasing to the eye 😛. Tbh I still am quite unsure as to why we shouldn't let users use the view they'd like to. Please do help me understand that. I can revert the commits once you let me know your thought process behind it.

As of documentation, we should definitely be updating it! We can focus on a sample minimal form that is necessary to make the gem usable.

[markets]

My reasoning is: the ERB view contains some logic (https://github.com/markets/sudo_rails/blob/master/app/views/sudo_rails/confirm_form.html.erb), so overriding this behavior is not the best idea for users, as you'll need to apply the same logic in your side and also, on each gem upgrade, it can potentially break. So, IMO the best way to match styles is to apply your app layout (it will pick your header, footer, fonts, colors, inputs, headings, ...) and if necessary, CSS overrides. But, if some "advanced users" wants to fully re-do the view, that's still possible by overriding the same view path in your app (as any Rails engine gem). I think that's the most common way to override views in gems, instead of exposing a custom "render".

[shettytejas]

Yea that makes sense. I'll update the PR.

[markets]

Thanks @shettytejas 🚀