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

Sort dashboard attributes #2133

Merged
merged 5 commits into from
Mar 7, 2022

Conversation

nhippenmeyer
Copy link
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
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
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
Member

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

@nhippenmeyer
Copy link
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
Member

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

@pablobm
Copy link
Collaborator

pablobm commented Feb 11, 2022

Agreed: id first, standard timestamps last.

@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
Contributor Author

@nickcharlton @pablobm mind taking another look?

@nickcharlton
Copy link
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?

spec/generators/dashboard_generator_spec.rb Show resolved Hide resolved
spec/generators/dashboard_generator_spec.rb Outdated Show resolved Hide resolved
spec/generators/dashboard_generator_spec.rb Outdated Show resolved Hide resolved
spec/generators/dashboard_generator_spec.rb Outdated Show resolved Hide resolved
@nhippenmeyer nhippenmeyer force-pushed the sort-dashboard-attributes branch from 3850cf2 to ae67bc1 Compare March 7, 2022 20:52
@nhippenmeyer
Copy link
Contributor Author

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

@pablobm
Copy link
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
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