Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Feature register pages #758

Closed
wants to merge 22 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

pcreux commented Nov 17, 2011

Hey @gregbell,

This branch is ready for review. Could you approve the refactorings I made?

I might add more scenarios to the feature registering_pages.

Cheers!

pcreux added some commits Nov 3, 2011

Add simple page registration.
It goes like:

ActiveAdmin.page "My Page" do

  content do
    h2 "Hey, this is a custom page"
    para "Awesome!"
  end

end

This spec file is getting very long, could we consider splitting this into namespace/register_page, namespace/register_resource, etc?

Owner

pcreux replied Nov 9, 2011

Yup! That's still WIP. I was thinking of using shared examples... Thanks for your feedback!

This looks awesome, can't wait to try it out!

Do you plan to add the ability to add a title to the page?

Owner

pcreux replied Nov 9, 2011

The title should be the same as the page name isn'it?

Yeah I think it should be, but when I tried to register a page no title or breadcrumb appeared. I do understand this is a WIP though :-)

Owner

pcreux replied Nov 9, 2011

Owner

pcreux commented on 6bd6114 Nov 16, 2011

The controller are refactored and they are quite nice to see. So we have:

::InheritedResources::Base 
    => AA::BaseController 
        => AA::PageController
        => AA::ResourceController 
            => AA::Dashboard
Owner

pcreux replied Nov 16, 2011

We could make Dashboard inheriting from PageController. However Dashboards will be deprecated sooner or later so it might not be worth it.

Those are not overwrites. I'm implementing the API of an active_admin_config.

So I wonder if I should make something like:

ActiveAdmin::Config
  => ActiveAdmin::Resource
  => ActiveAdmin::Page

@gregbell: Would that make sense?

Yes, this would make a lot of sense. I'm not that big of a fan of the name ActiveAdmin::Config, but I don't have any suggestions at this moment. There's definitely a lot of shared behaviour between Resource and Page though.

Owner

pcreux replied Nov 16, 2011

K, I'm on it!

pcreux added some commits Nov 16, 2011

Refactor Naming for Config and Resource!
Note: The documentation says that when registering a resource you can
set its name doing:

```
ActiveAdmin.register Post, :as => "Article"
```

The code however was made so that the :as resource would get a
plural. For instance:

```
ActiveAdmin.register Post, :as => "Articles"
```

I fixed this. It might be worth mentioning in the release notes.

@mattvague mattvague commented on the diff Nov 17, 2011

lib/active_admin/resource_controller.rb
@@ -5,23 +5,15 @@ require 'active_admin/resource_controller/callbacks'
require 'active_admin/resource_controller/collection'
require 'active_admin/resource_controller/filters'
require 'active_admin/resource_controller/form'
-require 'active_admin/resource_controller/menu'
-require 'active_admin/resource_controller/page_configurations'
require 'active_admin/resource_controller/scoping'
@mattvague

mattvague Nov 17, 2011

Contributor

Seems like it might be worth writing some documentation on what a ResourceController is for vs. a PageController

@mattvague mattvague commented on the diff Nov 17, 2011

lib/active_admin/page_controller.rb
@@ -0,0 +1,14 @@
+module ActiveAdmin
@mattvague

mattvague Nov 17, 2011

Contributor

Could we write some documentation of what Page is for? Kind of like the comments in resource.rb

@pcreux

pcreux Nov 18, 2011

Contributor

Yup! I'll add some comments!

@mattvague mattvague commented on the diff Nov 17, 2011

lib/active_admin/router.rb
end
end
+ when Page
+ match "/#{config.underscored_resource_name}" => "#{config.underscored_resource_name}#index"
@mattvague

mattvague Nov 17, 2011

Contributor

What if we wanted to have nested pages? E.G. /admin/stats/signups or something?

@pcreux

pcreux Nov 18, 2011

Contributor

Yep, why not! The point would be to make eye-candy URLs?

Maybe the DSL could support sub-pages. For instance:

ActiveAdmin.page "Stats" do
  content do
    # Stats page content
  end

  page "Signups" do
    content do
      # Stats/SIgnups page content
    end
  end

end

I think that this feature could be implemented later on though.

@mattvague

mattvague Nov 18, 2011

Contributor

Agreed, that'd be scope creep for now. But as for how it works i think you should have the option to define the sub page either in the parent page or in a seperate page in case it gets complex. Thoughts?

Contributor

pcreux commented Nov 18, 2011

@mattvague Thanks for reviewing this! :)

Owner

pcreux commented on 7dabab0 Nov 18, 2011

As per @mattvague request... :)

Owner

pcreux commented on 8bb90b2 Nov 18, 2011

@mattvague I hope this clarifies the code a bit.

We have an architecture like this:

Config => Resource
       => Page

BaseController => ResourceController
               => PageController

@gregbell I wonder if we could group those to make this hierarchy more obvious:

  • by type: lib/controllers and lib/configs
  • or by domain: lib/resources and lib/pages

@gregbell gregbell and 1 other commented on an outdated diff Nov 22, 2011

lib/active_admin/dsl.rb
@@ -117,6 +117,10 @@ module ActiveAdmin
controller.set_page_config :show, options, &block
end
+ def content(options = {}, &block)
+ controller.set_page_config :index, options, &block
+ end
@gregbell

gregbell Nov 22, 2011

Contributor

I'm not a big fan of parsing the config through DSL class. There are only two methods that are relavent to a Page, #content and #menu. I wonder if instead of using the DSL, we could just assume that the block is the content and the hash options can include the menu options.

@pcreux

pcreux Nov 23, 2011

Contributor

We now have PageDSL and ContentDSL. They both inherit from DSL.

I want customisation common to both DSL (such as menu or controller) similar.

@gregbell gregbell and 1 other commented on an outdated diff Nov 22, 2011

features/registering_pages.feature
@@ -0,0 +1,22 @@
+Feature: Registering Pages
+
+ Registering pages within Active Admin
+
+ Background:
+ Given I am logged in
+
+ Scenario: Registering a page
+ Given a configuration of:
+ """
+ ActiveAdmin.page "Status" do
@gregbell

gregbell Nov 22, 2011

Contributor

Instead of a method called #page, can we make it #register_page. This way it makes it obvious what it does and it's symmetrical to the #register method

@pcreux

pcreux Nov 22, 2011

Contributor

Yup! I'll do that!

@pcreux

pcreux Nov 23, 2011

Contributor

Done!

Contributor

pcreux commented Nov 23, 2011

Tested against a "real" rails app revealed a few regressions that I fixed. Also renamed #page method to #register_page and splitted DSL into ResourceDSL and PageDSL.

This feature is ready to be merged in.

An other thing we could do before releasing 0.4.0 is grouping classes by types in directories: controller, dsl, config.

Contributor

pcreux commented Nov 25, 2011

"This pull request cannot be automatically merged." Feck!

Contributor

simonoff commented Nov 25, 2011

This feature must be merged!

Hi
I've test your branch a bit and found issue with internalization.
Names of resources become non-latins if there are non-latin translations in active record locales.
result: namespace.rb:187:in `eval': (eval):1

line number is false actually. Error appears in "register_resource_controller" function

Contributor

gregbell commented Nov 26, 2011

I'll get this merged over the weekend. It's next on my list to merge :)

Contributor

pcreux commented Nov 26, 2011

@meskallito I didn't get what the issue is. What do you mean by non Latin translation?

Contributor

pcreux commented Nov 26, 2011

@gregbell I can fix the merge conflict if you want!

Functions in module "Naming" (class Resource < Config), that were rewritten by you in commit [f5c662f], provide resource names for controller_name in class Config. The problem occurs if I have models names translations in locales (for example in Chinese), because in that case "register_page_controller" tries to create controllers with non-latin names. is it clear?
Anyway, thank you for this functionality. It really helped me.

Contributor

pcreux commented Nov 26, 2011

Ok, that makes a lot of sense!

So the user facing names (resource.name and resource.plural_name) should
use i18n while the "system" ones (camelcased and underscored) should use...
the resource one?

For now you can't register pages with non-latin names then.

I will try to fix this before 11am GMT+1 (
http://thatz.at/Nov-27-2011-11-00-GMT+1-DST) so that this can get merged
into master before we release 0.4.0.

Contributor

pcreux commented Nov 26, 2011

@meskallito said:

Functions in module "Naming" (class Resource < Config), that were rewritten by you in commit [f5c662f], provide resource names for controller_name in class Config. The problem occurs if I have models names translations in locales (for example in Chinese), because in that case "register_page_controller" tries to create controllers with non-latin names. is it clear?
Anyway, thank you for this functionality. It really helped me.

Contributor

pcreux commented Nov 26, 2011

Ok, that makes a lot of sense!

So the user facing names (resource.name and resource.plural_name) should use i18n while the "system" ones (camelcased and underscored) should use... the resource one?

For now you can't register pages with non-latin names then.

I will try to fix this before 11am GMT+1 (http://thatz.at/Nov-27-2011-11-00-GMT+1-DST) so that this can get merged into master before we release 0.4.0.

Contributor

pcreux commented Nov 26, 2011

@gregbell I merged this branch into the current master and resolved 4 straight forward merge conflict.

It's available here: https://github.com/pcreux/active_admin/tree/merge-feature-register-pages

The feature registering_page does not pass though: http://travis-ci.org/#!/pcreux/active_admin/builds/344890

undefined local variable or method `active_admin_namespace' for #<ActiveAdmin::Views::HeaderRenderer:0xb8c2ee4> (ActionView::Template::Error)
493./lib/active_admin/arbre/builder.rb:47:in `method_missing'
494./lib/active_admin/renderer.rb:26:in `method_missing'
495./lib/active_admin/views/header_renderer.rb:16:in `title'

I let you taking care of this today. I won't have time to work on that this week-end.

FYI:
I also ran into this issue when running the specs on my machine (Ubuntu + ree 1.8.7):

Stylesheets when Rails 3.0.x should not have any syntax errors
     Failure/Error: css.should_not include("Syntax error:")
       expected "/*\nSyntax error: File to import not found or unreadable: bourbon.\n "
  [...]

Yes you are right. In addition, this functions affects the process of creating a common active_admin resources. Even if I dont init any page I'll get problems in register_resource_controller' cause it uses the same config.controller_name and in router.rb (it uses underscored_resource_name). I can prepapre pull request into your branch if you want

Contributor

pcreux commented Nov 26, 2011

@meskallito Sure, please do!

@pcreux ok, I'll do in an hour or two

Contributor

pcreux commented Nov 27, 2011

I just had to clean up my test env to get the spec to pass.

And I won't fix the non-latin support this week-end.

Contributor

gregbell commented Nov 28, 2011

I rebased the branch off master and then merged it in. There are definitely some things I would like to add before releasing this with 0.4.0 such as documentation, sidebar items, action items and the issue that @meskallito is having.

Nice work @pcreux! This is an awesome feature.

@gregbell gregbell closed this Nov 28, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment