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

Dashboard magic? #460

Closed
micapam opened this issue Feb 12, 2016 · 14 comments
Closed

Dashboard magic? #460

micapam opened this issue Feb 12, 2016 · 14 comments

Comments

@micapam
Copy link

micapam commented Feb 12, 2016

I'm wondering why the dashboards have to declare all the attribute types explicitly. Can't these be inferred from the underlying model, and only declared if they are to be overriden?

I know we don't need to key these in as they are created by the generator, but it would be nice if the generated dashboard class could be less verbose. And it would be more self-documenting to have an ATTRIBUTE_TYPE_OVERRIDES hash as you could see at a glance which ones aren't the default field types.

If you agree with what I'm saying I'd be happy to raise a PR. Just wanted to know if you guys can think of a reason the approach I'm suggesting wouldn't work or isn't a good idea...

@micapam
Copy link
Author

micapam commented Feb 12, 2016

Another way to make the dashboards less verbose would be, instead of three *_ATTRIBUTES arrays, to have a single array with hashes e.g.

ATTRIBUTES = [
  foo: { :collection, :show, :form },
  bar: { :show, :form },
  immutable: { :show }
]

or maybe:

attribute :foo # implicitly, everywhere (hmmm....)
attribute :bar, except: :collection
attribute :immutable, only: :show 

or maybe even combine with attribute type overrides:

attribute :foo # implicitly, hidden (same as access: none)
attribute :poo, access: :all
attribute :bar, access_except: :collection
attribute :immutable, access_only: :show
attribute :bizarro, type: Field::Funky, access: :all

@eddietejeda
Copy link
Contributor

I agree with this. Editing different variables and keeping track of what fields are defined in what variable is cumbersome.

I like all your examples, especially the last one. It resembles the Rails models convention most.

@c-lliope
Copy link
Contributor

well, kind of expected this. 😝

One of the reasons that Administrate is easy to understand and work with is the "No DSLs" guiding principle we started out with (in the README). These proposals would go against that. I'm not entirely opposed to this idea, but I think we should have a significantly pressing reason to break that rule.

We've had discussions about automatically detecting the field type, and decided against it because it would introduce a bit of maintenance overhead. The most notable pitfall is that Administrate would break between major Rails version bumps, because it would be relying on internal API methods to detect the field types

I last talked with @sgrif about this, maybe he has thoughts.

@micapam
Copy link
Author

micapam commented Feb 15, 2016

I guess the main reason I thought of this change is that the current version doesn't support namespaced models - so I have ended up keying in my own dashboards on my own fork based on @gpinkham's changes. Once administrate does support namespaced models I suppose this will be less of a problem as the generators will take care of it.

I do think the dashboards are quite noisy, though - the generated classes are basically just clusters of data. Another way to approach this without breaking your anti-DSL stance - which I do sympathise with, having hated on ActiveAdmin for a long time! - would be to push this stuff into config.

# config/administrate/my-model.yaml
attributes: 
  foo: 
    field:
      type: string
    access: 
      - collection
      - show
      - form
  bar: 
    field:
      type: number
      options: 
        title: 'Unit Price'
        prefix: '$'
        decimals: 2
        multiplier: 0.0.1
    access: 
      - collection
      - show
# etc

... but maybe you would feel like this YAML is a DSL in disguise, haha! :)

@micapam micapam closed this as completed Mar 3, 2016
@micapam
Copy link
Author

micapam commented Mar 3, 2016

Closing this as I guess there's not much interest and I would prefer attention to be paid to my bug issue: #471

@c-lliope
Copy link
Contributor

c-lliope commented Mar 8, 2016

I'm coming back to this because I think it'll make some of the other issues more feasible to implement. Here's what I'm thinking for a first pass:

  ALL_PAGES = [:show, :index, :new, :edit]
  FORM_PAGES = [:new, :edit]

  attribute :created_at, Field::DateTime, [:show, :index]
  attribute :email, Field::Email, ALL_PAGES
  attribute :email_subscriber, Field::Boolean, ALL_PAGES
  attribute :lifetime_value, Field::Number, [:show, :index], prefix: "$", decimals: 2
  attribute :name, Field::String, FORM_PAGES
  attribute :orders, Field::HasMany, ALL_PAGES
  attribute :updated_at, Field::DateTime, [:show, :index]

Hopefully, this approach would let us turn dashboard objects into wrappers around the underlying database models. The attributes would define which methods are available on the dashboard objects.

This would be similar to the API that ActiveModel::Serializers uses.

@c-lliope c-lliope reopened this Mar 8, 2016
@abousamra
Copy link
Contributor

I really like that. This approach will give it much more flexibility.

@eddietejeda
Copy link
Contributor

Looks good to me! 👍

@ldeld
Copy link

ldeld commented May 21, 2018

@Graysonwright Has there been any update on this? I did not find any references to the new syntax you proposed in the documentation

@bhanuone
Copy link

Looks like this project was abandoned.

@gpinkham
Copy link

@bhanuone they have had commits this month and many in the last few months so seems like it's active.

@bhanuone
Copy link

@gpinkham Can't say the same looking at the number of PRs pending and age of the Issues.

@gpinkham
Copy link

Good point.

@pablobm
Copy link
Collaborator

pablobm commented Dec 12, 2019

Hello all. I personally am not a big fan of this proposal:

  1. It doesn't change the behaviour of Administrate. In other words: in terms of the end user, it doesn't add anything that the current API can't achieve.
  2. It introduces complexity.
  3. It creates new edge cases. For example: what if I want fields to appear in a different order across index, show and the forms?
  4. It can be implemented as a third-party plugin.

Re: the third-party plugin, I think it's possible to create a mixin to Administrate::BaseDashboard that implements this DSL. This way:

  1. We keep this complexity away from the base gem.
  2. It's possible to explore different alternatives to this syntax without affecting Administrate, by evolving the plugin.
  3. Those of you who so wish can use it.

I understand this is not a perfect solution. A third-party gem is not necessarily going to be maintained as well as the official gem. On the other hand, adding functionality to Administrate makes maintenance more difficult, affecting all issues and PRs across the board. We are already slower that I'd wish. Lately I'm getting time to work through the backlog, but this will not last forever.

If someone creates such a plugin, I'm happy to mention it on the documentation, to help people become aware of it.

Thoughts? (Also: /cc @GraceWasHere @nickcharlton)

@pablobm pablobm closed this as completed Dec 12, 2019
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

No branches or pull requests

9 participants