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

Generate SHOW_PAGE_ATTRIBUTES as an explicit array #130

Closed
mcmire opened this issue Nov 2, 2015 · 4 comments
Closed

Generate SHOW_PAGE_ATTRIBUTES as an explicit array #130

mcmire opened this issue Nov 2, 2015 · 4 comments
Assignees

Comments

@mcmire
Copy link

mcmire commented Nov 2, 2015

Right now, COLLECTION_ATTRIBUTES, SHOW_PAGE_ATTRIBUTES, and FORM_ATTRIBUTES use ATTRIBUTE_TYPES.keys as a base set of attributes. It would follow, then, that in order to re-order fields that appear in a form or table, you re-order the keys in ATTRIBUTE_TYPES. However, this seems a little odd, because ATTRIBUTE_TYPES is a hash, and so the order of its keys should be irrelevant. (It just so happens that hashes in Ruby are now ordered, but they really shouldn't be, and we shouldn't be assuming they are.)

So here's a suggestion: introduce a BASE_ATTRIBUTES constant that all of the other *_ATTRIBUTES constants use. The dashboard generator would have to set this to an explicit array based on the columns the model has, but then we'd encourage people to modify this array instead of ATTRIBUTE_TYPES. While we're at it, it might be useful to place ATTRIBUTE_TYPES first.

So a dashboard class would look like this:

ATTRIBUTE_TYPES = {
  author: Field::BelongsTo.with_options(class_name: "User"),
  content: Field::Text,
  id: Field::Number,
  ...
}

BASE_ATTRIBUTES = [
  :author,
  :content,
  :id,
  ...
]

READ_ONLY_ATTRIBUTES = [
  :id,
  :created_at,
  :updated_at
]

TABLE_ATTRIBUTES = BASE_ATTRIBUTES.first(4)

SHOW_PAGE_ATTRIBUTES = BASE_ATTRIBUTES

FORM_ATTRIBUTES = BASE_ATTRIBUTES - READ_ONLY_ATTRIBUTES
@c-lliope
Copy link
Contributor

c-lliope commented Nov 6, 2015

hm... I originally moved away from a similar solution, with ATTRIBUTE_TYPES.keys and READ_ONLY_ATTRIBUTES, because I thought it would be confusing what was part of the API and what wasn't.

That was in #111. What do you think?

I do like how this solution has a single place to change the order, but I'm not sure if it'll make things difficult for new users.

@mcmire
Copy link
Author

mcmire commented Nov 6, 2015

Two points about that:

  • Since SHOW_PAGE_ATTRIBUTES uses the keys from ATTRIBUTE_TYPES, you're still abusing a hash and assuming its keys are ordered.
  • Other than SHOW_PAGE_ATTRIBUTES, is ATTRIBUTE_TYPES now used at all? That's unclear from the changes.

@c-lliope
Copy link
Contributor

@mcmire I agree about the hash.

All of the constants in that file are required by Administrate, so yes - ATTRIBUTE_TYPES is used by Administrate.

I think I'd prefer to spell out SHOW_PAGE_ATTRIBUTES as an array, similar to how TABLE_ATTRIBUTES and FORM_ATTRIBUTES are now.

It would be up to the user to abstract duplication where it makes sense.

Thoughts?

@mcmire
Copy link
Author

mcmire commented Nov 19, 2015

I think that's fine. I think that the attributes are going to end up getting customized anyway and in order to do so it's best to have it as an array.

@c-lliope c-lliope added this to the v0.1.2 milestone Nov 19, 2015
@c-lliope c-lliope added the ready label Dec 15, 2015
@c-lliope c-lliope changed the title Suggestion: Introduce a BASE_ATTRIBUTES constant Generate SHOW_PAGE_ATTRIBUTES as an explicit array Dec 15, 2015
c-lliope added a commit that referenced this issue Dec 21, 2015
Closes #130

Problem:

The dashboard generator specifies:

```ruby
SHOW_PAGE_ATTRIBUTES = ATTRIBUTE_TYPES.keys
```

This means that the order of the show page attributes
is dependent on the order of keys in the `ATTRIBUTE_TYPES` hash.

Ruby hashes happen to be ordered,
but we shouldn't make an assumption that hashes are ordered
because that is not a property that hashes are designed to have.

Solution:

Because we assume that users will be editing `SHOW_PAGE_ATTRIBUTES`
to customize the contents and order of show page attributes,
we'll generate an explicit array for them to modify.
@c-lliope c-lliope self-assigned this Dec 22, 2015
c-lliope added a commit that referenced this issue Jan 8, 2016
Closes #130

Problem:

The dashboard generator specifies:

```ruby
SHOW_PAGE_ATTRIBUTES = ATTRIBUTE_TYPES.keys
```

This means that the order of the show page attributes
is dependent on the order of keys in the `ATTRIBUTE_TYPES` hash.

Ruby hashes happen to be ordered,
but we shouldn't make an assumption that hashes are ordered
because that is not a property that hashes are designed to have.

Solution:

Because we assume that users will be editing `SHOW_PAGE_ATTRIBUTES`
to customize the contents and order of show page attributes,
we'll generate an explicit array for them to modify.
@c-lliope c-lliope removed the ready label Jan 8, 2016
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

2 participants