-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add Global mode to CategoryWidget & PieWidget #370
Add Global mode to CategoryWidget & PieWidget #370
Conversation
This pull request has been linked to Shortcut Story #219449: CategoryWidget & PieWidget - global (remote) mode. |
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.
LGTM
: formatOperationColumn(operationColumn || column, joinOperation) | ||
}) as value`; | ||
|
||
return `SELECT COALESCE(${column}, 'null') as name, ${selectValueClause} FROM ${formatTableNameWithFilters( |
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.
Just to confirm, what databases should support this feature?
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.
Any of them. Do you know any incompatibility here?
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.
Nop, but it would be nice to involve Data team in these queries verification. Can you gather them (not just this one) and pass them to @Jesus89 please?
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.
LGTM
@@ -10,7 +10,7 @@ export default function useWidgetFetch( | |||
{ id, dataSource, params, global, onError } | |||
) { | |||
// State | |||
const [data, setData] = useState(null); | |||
const [data, setData] = useState(); |
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.
Why this change here Sergio?
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.
As you can see in that line, we define a default value. A default value can only be assigned in destructuring when it's undefined, not null.
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.
LGTM, just add changelog entry please
Let's coordinate to see how we can merge (& test) a group of several widgets together. Most probably: Formula, Category and Histogram first
… of github.com:CartoDB/carto-react into feature/sc-219449/categorywidget-piewidget-global-remote-mode
… of github.com:CartoDB/carto-react into feature/sc-219449/categorywidget-piewidget-global-remote-mode
4a6fbdb
into
feature/sc-219448/formulawidget-remote-mode-for-global
No description provided.