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

Improve default resource display #191

Merged
merged 1 commit into from
Nov 12, 2015
Merged

Improve default resource display #191

merged 1 commit into from
Nov 12, 2015

Conversation

c-lliope
Copy link
Contributor

@c-lliope c-lliope commented Nov 8, 2015

Closes #129

Problem:

Administrate relies on #to_s
to display resources throughout the system.
In order to get Administrate working correctly,
developers must define #to_s on all models
that will be displayed in the admin dashboard.

This process is not documented,
can be confusing,
and gets in the way of a zero-configuration dashboard.

Solution:

Add Administrate::BaseDashboard#display_resource,
which takes a resource and returns a sensible string representation.
Devs can overwrite this method on a per-dashboard basis
to customize how their resources are displayed.

In order to document this method,
we generate comments in each dashboard class that follow the format:

  # Overwrite this method to customize how line items are displayed
  # across all pages of the admin dashboard.
  #
  # def display_resource(line_item)
  #   "LineItem ##{line_item.id}"
  # end

TODO

  • Add comments to generated dashboards
  • CHANGELOG

@@ -35,5 +35,9 @@ def show_page_attributes
def collection_attributes
self.class::COLLECTION_ATTRIBUTES
end

def display_resource(resource)
"#{resource.class.to_s} ##{resource.id}"

Choose a reason for hiding this comment

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

Redundant use of Object#to_s in interpolation.

@marisawallace
Copy link

Looks good to me. Using inheritance seems worthwhile in this case... after adding to_s methods to my models, I found that most of them followed a common format.

@c-lliope c-lliope added this to the 0.1.1 milestone Nov 8, 2015
@mcmire
Copy link

mcmire commented Nov 9, 2015

This seems reasonable to me. I've kind of thought overriding to_s felt a little dirty.

@@ -16,5 +16,5 @@ By default, the relationship is rendered as a link to the associated object.
%>

<% if field.data %>
<%= link_to field.data, [Administrate::NAMESPACE, field.data] %>
<%= link_to field.associated_resource, [Administrate::NAMESPACE, field.data] %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change part of removing the dependency on to_s ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep - Rails was taking field.data and turning it into field.data.to_s, in order to get the link text.

I'm not sure associated_resource is super clear. I might try changing it to display_associated_resource or something.

Copy link

Choose a reason for hiding this comment

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

If I were to describe this in English, it'd be "the human-readable representation of the resource that this field points to".

So display_associated_resource is kind of saying the same thing. But I can't think of a better name, honestly.

It would be nice if associated_resource were a decorator around a record instead of a record itself. Actually, every resource that needs to be displayed could be decorated, and then you could have your to_s method again, only inside the decorator class. I guess it wouldn't be easy for a user to override that, though.

Copy link

Choose a reason for hiding this comment

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

Which one looks better:

  • display_associated_resource
  • displayed_associated_resource

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like displayed_..., because it's a noun instead of a verb. The method is returning a String, instead of taking action to display the string itself.

@JoelQ
Copy link
Contributor

JoelQ commented Nov 9, 2015

👍

:to_s,
) %>
<%= f.select(field.permitted_attribute) do %>
<%= options_for_select(field.candidate_options) %>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

candidate_resource_options, maybe?

Copy link

Choose a reason for hiding this comment

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

I feel like the word candidate is redundant here, given that you already have option in the same word. In other words, something is either a candidate or an option, but being both looks strange.

What about display_associated_resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep - I was using option in the sense of the <option> tag for <select> fields, but I see what you mean. I think I like associated_resource_options.

Problem:

Administrate relies on `#to_s`
to display resources throughout the system.
In order to get Administrate working correctly,
developers must define `#to_s` on all models
that will be displayed in the admin dashboard.

This process is not documented,
can be confusing,
and gets in the way of a zero-configuration dashboard.

Solution:

Add `Administrate::BaseDashboard#display_resource`,
which takes a resource and returns a sensible string representation.
Devs can overwrite this method on a per-dashboard basis
to customize how their resources are displayed.

Add generated comment for `display_resource`, that looks like:

```
  # Overwrite this method to customize how line items are displayed
  # across all pages of the admin dashboard.
  #
  # def display_resource(line_item)
  #   "LineItem ##{line_item.id}"
  # end
```

Minor changes:

Extract `Administrate::Field::Associative` as a superclass,
which contains logic for looking up associated dashboards
and associated models' classes.

Fix PR suggestions
@c-lliope c-lliope force-pushed the gw-display-resource branch from eb0cb2d to fcd3fc7 Compare November 12, 2015 07:15
@c-lliope c-lliope merged commit fcd3fc7 into master Nov 12, 2015
@c-lliope c-lliope deleted the gw-display-resource branch November 12, 2015 07:21
c-lliope added a commit that referenced this pull request Nov 12, 2015
- Update gem version
- Update README with recommended optimistic versioning for bundler
- Update README with warning about Administrate's pre-1.0 status
- Update CHANGELOG to fill in missing PR references
- sort CHANGELOG entries according to change type

Changes:

```
* [#191] [CHANGE] Improve API for specifying how resources are displayed
  across the dashboard.
  * Models are now displayed with a sensible default - (e.g. "User #2")
  * Users can define `ModelDashboard#display_resource(resource)` for custom
    display behavior
  * Users who have generated views for the following field types
    may need to update them to take advantage of the new API:
    * HasOne
    * HasMany
    * Polymorphic
    * BelongsTo
* [#223] [FEATURE] Translation: Vietnamese
* [#161] [FEATURE] Translation: Mandarin Chinese
* [#196] [FEATURE] Translation: Taiwanese Mandarin
* [#142] [FEATURE] Translation: Brazilian Portuguese
* [#171] [FEATURE] Translation: Polish
* [#153] [FEATURE] Translation: Russian
* [#148] [FEATURE] Translation: French
* [#147] [FEATURE] Translation: German
* [#154] [FEATURE] Translation: Spanish
* [#126] [UI] Preserve whitespace when rendering text fields
* [#194] [BUGFIX] Don't clear out datetime values in form fields
* [#193] [BUGFIX] Don't assume that unrecognized db column types are searchable
* [#124] [BUGFIX] Better detection of application models
* [#156] [COMPAT] Include missing `sass-rails` dependency in gemspec
* [#174] [COMPAT] Make several missing dependencies explicit.
* [#144] [COMPAT] Update repository structure so Bundler can pull the gem from github.
  (e.g. `gem "administrate", github: "thoughtbot/administrate"`)
* [#166] [COMPAT] Use ANSI SQL standards for case-insensitive search
* [#120] [DOC] Add Rubygems version badge to README
* [#165] [DOC] Add CircleCI badge to README
* [#119] [DOC] Add CodeClimate badge to README
```
c-lliope added a commit that referenced this pull request Nov 12, 2015
- Update gem version
- Update README with recommended optimistic versioning for bundler
- Update README with warning about Administrate's pre-1.0 status
- Update CHANGELOG to fill in missing PR references
- add `[I18n]` category to CHANGELOG
- sort CHANGELOG entries according to change type

Changes:

* [#191] [CHANGE] Improve API for specifying how resources are displayed
  across the dashboard.
  * Models are now displayed with a sensible default - (e.g. "User #2")
  * Users can define `ModelDashboard#display_resource(resource)` for custom
    display behavior
  * Users who have generated views for the following field types
    may need to update them to take advantage of the new API:
    * HasOne
    * HasMany
    * Polymorphic
    * BelongsTo
* [#223] [FEATURE] Translation: Vietnamese
* [#161] [FEATURE] Translation: Mandarin Chinese
* [#196] [FEATURE] Translation: Taiwanese Mandarin
* [#142] [FEATURE] Translation: Brazilian Portuguese
* [#171] [FEATURE] Translation: Polish
* [#153] [FEATURE] Translation: Russian
* [#148] [FEATURE] Translation: French
* [#147] [FEATURE] Translation: German
* [#154] [FEATURE] Translation: Spanish
* [#126] [UI] Preserve whitespace when rendering text fields
* [#194] [BUGFIX] Don't clear out datetime values in form fields
* [#193] [BUGFIX] Don't assume that unrecognized db column types are searchable
* [#124] [BUGFIX] Better detection of application models
* [#156] [COMPAT] Include missing `sass-rails` dependency in gemspec
* [#174] [COMPAT] Make several missing dependencies explicit.
* [#144] [COMPAT] Update repository structure so Bundler can pull the gem from github.
  (e.g. `gem "administrate", github: "thoughtbot/administrate"`)
* [#166] [COMPAT] Use ANSI SQL standards for case-insensitive search
* [#120] [DOC] Add Rubygems version badge to README
* [#165] [DOC] Add CircleCI badge to README
* [#119] [DOC] Add CodeClimate badge to README
@tute
Copy link
Contributor

tute commented Nov 17, 2015

🎉 😃

@wspurgin
Copy link

@Graysonwright Any chance this change can be added to documentation somewhere? I recently updated an old system's administrate, and it took me a long time to find this change.

@c-lliope
Copy link
Contributor Author

c-lliope commented Mar 7, 2016

@wspurgin - the change is listed in the 0.1.1 release notes, but I guess it's not very visible. I'll change up the format of release notes, and add a link to them from the README.

Thanks for the feedback!

@c-lliope
Copy link
Contributor Author

c-lliope commented Mar 7, 2016

See #509.

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.

7 participants