-
Notifications
You must be signed in to change notification settings - Fork 654
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
geometry types frontend [do not merge] #2189
Conversation
Frontend tests were OK 👍 (details) |
@@ -259,20 +249,7 @@ cdb.admin.CartoDBTableData = cdb.ui.common.TableData.extend({ | |||
var self = this; | |||
// Unlink schema binding avoiding | |||
// double sql requests | |||
this.unlinkFromSchema(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the comment above (Unlink schema binding avoiding double sql requests) be deleted as well since this method call was removed? it's not obvious from the new code if that still applies :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Apart from the comments 👍 |
geometry_types: view.getGeometryTypes() | ||
}); | ||
}, this); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excuse my backbone.js ignorance: my understanding is that this is binding a piece of code to the reset
event of the CartoDBTableMetadata
model and it will be called upon fetching data from the server. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, every time we fetch data from sql api we will update schema and geometry types in CartoDBTableMetadata
@rafatower could you merge this branch to #2165 and put in staging to tests it cc @iriberri @Xatpy |
sure! |
Frontend tests were OK 👍 (details) |
work being done on #2165 |
Frontend tests were OK 👍 (details) |
@rafatower could you merge and deploy to staging again? |
done! |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
geometry types fetched form SQL api frontend side
Frontend tests were OK 👍 (details) |
Frontend tests were OK 👍 (details) |
frontend needed the geometry_types and schema to be served from rails api. This patch changes that and ask to SQL for schema and geometry types
This was being done when the user applied a SQL to a layer so the change has been "as easy" as move the code to the right place so all the requests are done in the same way
In the way I removed some dependecies between table metadata and table data.
This needs smoke tests