close
Skip to content

Sort dashboard attributes#2133

Merged
pablobm merged 5 commits into
thoughtbot:mainfrom
rinsed-org:sort-dashboard-attributes
Mar 7, 2022
Merged

Sort dashboard attributes#2133
pablobm merged 5 commits into
thoughtbot:mainfrom
rinsed-org:sort-dashboard-attributes

Conversation

@nhippenmeyer
Copy link
Copy Markdown
Contributor

Sorting the fields listed in the ATTRIBUTE_TYPES hash of the dashboard file will make these files easier to maintain. For dashboards that contain a large number of attributes, it can be time-consuming/inefficient to find the correct line when making a change.

@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch 2 times, most recently from b2ec977 to 92b1437 Compare January 26, 2022 17:45
@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch from 92b1437 to a3fb4da Compare January 26, 2022 17:46
Copy link
Copy Markdown
Member

@nickcharlton nickcharlton left a comment

Choose a reason for hiding this comment

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

Thanks for this!

I think if we're going to sort one of them, we should also sort the rest of the dashboard attributes. What do you think?

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Feb 10, 2022

I feel that id should go first by default. Thoughts?

Anecdotically, at work there's an admin interface I often have to look at (not Administrate-based) where everything sorted alphabetically, and I often end up scrolling right for a while until I find the ids...

@nickcharlton
Copy link
Copy Markdown
Member

Yeah, I like that. It felt weird having id somewhere in the middle some times…

@nhippenmeyer
Copy link
Copy Markdown
Contributor Author

nhippenmeyer commented Feb 11, 2022

I think if we're going to sort one of them, we should also sort the rest of the dashboard attributes. What do you think?

I'm happy to sort the attributes in COLLECTION_ATTRIBUTES, SHOW_PAGE_ATTRIBUTES and FORM_ATTRIBUTES as well, however I feel less strongly about that since those lists are typically reordered manually anyway based on the requirements of each dashboard.

I feel that id should go first by default. Thoughts?

I agree that id is more of special-case attribute and could make sense to put it first in the list. How do you feel about where created_at and updated_at should appear? Should they be at the bottom or sorted alphabetically with the rest of the attributes?

@nickcharlton
Copy link
Copy Markdown
Member

Good point! We should put created_at/updated_at at the bottom, I think.

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Feb 11, 2022

Agreed: id first, standard timestamps last.

Comment thread spec/generators/dashboard_generator_spec.rb
@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch 3 times, most recently from 4f4841d to 0d311d9 Compare February 11, 2022 19:00
@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch from 0d311d9 to c11bea1 Compare February 11, 2022 20:26
@nhippenmeyer
Copy link
Copy Markdown
Contributor Author

@nickcharlton @pablobm mind taking another look?

@nickcharlton
Copy link
Copy Markdown
Member

Ah hah, yeah, that works as I was hoping. I'm pondering how we can simplify the implementation, but I'm not getting anywhere today, any thoughts, @pablobm?

Comment thread spec/generators/dashboard_generator_spec.rb
Comment thread lib/generators/administrate/dashboard/dashboard_generator.rb Outdated
Comment thread spec/generators/dashboard_generator_spec.rb Outdated
Comment thread spec/generators/dashboard_generator_spec.rb Outdated
Comment thread spec/generators/dashboard_generator_spec.rb
@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch from 3850cf2 to ae67bc1 Compare March 7, 2022 20:52
@nhippenmeyer
Copy link
Copy Markdown
Contributor Author

@nickcharlton @pablobm I've made your suggested improvements. Does this look good now?

@pablobm
Copy link
Copy Markdown
Collaborator

pablobm commented Mar 7, 2022

Nice, let's merge this bad boi! :shipit:

@pablobm pablobm merged commit ccb3060 into thoughtbot:main Mar 7, 2022
@shadoath shadoath deleted the sort-dashboard-attributes branch March 11, 2025 21:05
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.

3 participants