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

Move gpx_file (trace) xml generation out of model and into view #2207

Merged
merged 2 commits into from
Jun 26, 2019
Merged

Move gpx_file (trace) xml generation out of model and into view #2207

merged 2 commits into from
Jun 26, 2019

Conversation

woodpeck
Copy link
Contributor

This continues the work from #2164 for the "trace" class.

@tomhughes tomhughes changed the title move gpx_file (trace) xml generation out of model and into view Move gpx_file (trace) xml generation out of model and into view Apr 14, 2019
@tomhughes
Copy link
Member

tomhughes commented Apr 14, 2019

This all fine for the most part but it does create a slightly confusing naming situation.

We wind up with a trace view that actually takes an argument named @traces and renders multiple traces so that it can also support a separate action on the user controller.

One option would be just to rename it to traces so that things are consistent, but that means we have a view that doesn't directly relate to any action. The only other approach I can see is to have separate top level views though they could still share the common partial where most of the work is done.

@gravitystorm
Copy link
Collaborator

The only other approach I can see is to have separate top level views though they could still share the common partial where most of the work is done.

That would be my preferred approach. Each action (show, data, index, etc) should have a correspondingly named view, even if that view is little more than including the partial. This keeps everything where a new developer would expect to find it.

@gravitystorm
Copy link
Collaborator

I've added another commit to this PR based on Tom's feedback and my own, and I think it's good to go now.

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