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

Refactor Comments UI #2113

Merged
merged 1 commit into from
Apr 19, 2013
Merged

Refactor Comments UI #2113

merged 1 commit into from
Apr 19, 2013

Conversation

seanlinsley
Copy link
Contributor

While working on #1979, I noticed that we happen to register an index page for Comments, but it's hidden by default. This PR updates that UI so it's worthy of being shown by default.

Changes
  • new configuration options:
  • index page:
    • added a scope for every AA namespace, with the current namespace as the default
    • updated resource_type and author_type filters to be select fields (for Rails >= 3.2)
    • updated table to show every attribute
  • show page:
    • instead of redirecting to the associated resource, we now have a proper show page

@seanlinsley
Copy link
Contributor Author

@macfanatic since I went to the trouble of modernizing the Comments index view, I'd really like it to be usable. By that, I mean the menu visibility should be a config setting, both for the application and the namespace level. While I'm at it, the as: "AdminComment should also be configurable.

What do you think?

@macfanatic
Copy link
Contributor

I'm not sure what you are getting at with your two examples of what it should be able to do. Can you elaborate?

@seanlinsley
Copy link
Contributor Author

Both with the current state of this PR, and with master, you can go to /admin_comments and get an index page showing all comments. The problem is, the menu item for this is turned off. I want that to be configurable.

An earlier PR also changed as: "Comment" to as: "AdminComment" because AA was basically making Comment a reserved word. That PR was just a workaround; a real fix would be to make the name under which admin comments are registered configurable.

@macfanatic
Copy link
Contributor

Gotcha, makes sense. I'm not positive, but I think you could just use the inherittable_setting method on ActiveAdmin::Application ( I believe ) to achieve what you want, with settings per namespace or globally exposed in the config/initializers/active_admin.rb file.

@seanlinsley
Copy link
Contributor Author

Yeah, it's been done plenty of times in the AA codebase. Just wanted to get some feedback.

@seanlinsley
Copy link
Contributor Author

Ah... neither uniq nor pluck existed before Rails 3.2...

@seanlinsley
Copy link
Contributor Author

Okay, I've mostly finalized this PR. Everything seems to work right.

@seanlinsley
Copy link
Contributor Author

The cukes are failing; I'll fix them tomorrow :V

@seanlinsley
Copy link
Contributor Author

All green 🍀. Anything you want changed @macfanatic? BTW, I'll want to squash these commits before they're merged.

@macfanatic
Copy link
Contributor

Squash and I'll merge, looks good to me.

+ new configuration options:
  + menu item visibility (defaults to true)
  + `:as` registration name. For #301; replaces #2060 (defaults to Comment)
+ index page:
  + added a scope for every AA namespace, with the current namespace as the default
  + updated `resource_type` and `author_type` filters to be select fields (for Rails >= 3.2)
  + updated table to show every attribute
+ show page:
  + instead of redirecting to the associated resource, we now have a proper show page
@seanlinsley
Copy link
Contributor Author

Okay, squashed.

macfanatic added a commit that referenced this pull request Apr 19, 2013
@macfanatic macfanatic merged commit a87a939 into activeadmin:master Apr 19, 2013
@seanlinsley seanlinsley deleted the refactor/comments_ui branch April 19, 2013 19:33
@coreyward
Copy link
Contributor

By the way, as a result of a Comments admin heading being automatically added, I straight up disabled comments. I don't know why my clients want to see a CRUD interface for admin comments on records in the admin.

Update: Apparently you can't disable comments. It hides the heading, but the queries are still made.

@seanlinsley
Copy link
Contributor Author

There are three settings. You'd need to use the first one.

config.allow_comments             # outright disables comments
config.show_comments_in_menu      # hides the menu item
config.comments_registration_name # lets you use e.g. AdminComment instead of Comment

@coreyward
Copy link
Contributor

I actually used the first one, but had active_admin_comments in a show block, which caused Ruby to make the queries and hang. See #2153.

@seanlinsley
Copy link
Contributor Author

Looking into it now.

So you're aware, before this PR Comments were already fully registered as a AA resource. As in if you went to /comments/ in your browser, you'd arrive at the hidden index page.

This PR was an attempt to make that page actually be useful. If the end result of me bringing this to light is that Comments are modularized and removed from AA proper, I'm fine with that. This was more of an effort to understand the current codebase and to try and improve it where possible.

@coreyward
Copy link
Contributor

All of these hidden features in ActiveAdmin are really more of a pain than a boon, so I appreciate your exposing it

I have tried (and failed) to build clean installs without comments in the past but because ActiveAdmin is so tightly coupled to the notion of including comments I can never manage to get it to work properly without creating the comments tables at the least.

Are the two flags you pointed out above (show_comments_in_menu and comments_registration_name) in the documentation and included in newly generated active_admin.rb configs?

Random idea for consideration — why not generate the comment resource when AA is installed so users can see/modify/delete it?

@seanlinsley
Copy link
Contributor Author

I haven't added them to the docs, but they are included in newly generated AA config files (see the diff of this PR)

@seanlinsley
Copy link
Contributor Author

Coming back to this thread...

@coreyward the one thing I'm aware of that would prevent moving Comments completely into the realm of the everyday developer would be registration of the view helper, active_admin_comments, that's used on the show page. If Comments were added to your app by the AA generator, then we would need to also move the view helper. Any ideas on where in the app directory structure we should put it?

@abrambailey
Copy link

abrambailey commented Jan 23, 2017

I know this is very old, and by the way hello @coreyward, but apparently I missed the ability to change the registered names for comments using config.comments_registration_name = 'AdminComment' (posting this comment in case someone else is digging around looking for the solution).

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.

4 participants