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

3879 new named maps fields #3974

Merged
merged 14 commits into from
Jun 10, 2015
Merged

3879 new named maps fields #3974

merged 14 commits into from
Jun 10, 2015

Conversation

Kartones
Copy link
Contributor

@Kartones Kartones commented Jun 8, 2015

No description provided.

@@ -137,6 +137,7 @@ def qualified_name(viewer_user=nil)
end
end

# Despite storing always a named map, no need to retrievfe it for "public" visualizations
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo.

end

data = {
zoom: visualization.map.zoom,
Copy link
Contributor

Choose a reason for hiding this comment

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

This method has too many visualization.map.whatever. Delegation (visualization.whatever) is not a good solution here, but probably you could move most of the method (or all) to Map and use self instead of receiving an object.

@juanignaciosl
Copy link
Contributor

As I've said in the comments, I'd move many of class method Visualization#view_data_from to instance methods in Visualization, Map and Layer (or their presenters, not sure). But it's just a matter of style. 👍 if you don't think the same.

@Kartones
Copy link
Contributor Author

Agreed on moving to 'Map' this methods, they are still ugly as hell but now is not the time to refactor how Map attributes are stored so convenience methods will do 👍

Presenters I don't like because the issue is that right now stored Map attributes are suboptimal (a string), where it should have real numbers and if something, have a presenter add the string version (but here I don't see it as is just the same data inside an array).

Kartones added a commit that referenced this pull request Jun 10, 2015
@Kartones Kartones merged commit f44d911 into master Jun 10, 2015
@Kartones Kartones deleted the 3879-new-named-maps-fields branch June 10, 2015 13:35
@Kartones Kartones restored the 3879-new-named-maps-fields branch June 10, 2015 14:54
@Kartones Kartones deleted the 3879-new-named-maps-fields branch August 26, 2015 14:49
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.

2 participants