677 popup api #682
677 popup api #682
Conversation
cartoframes/vis/popup.py
Outdated
| output = [] | ||
| for item in array: | ||
| if isinstance(item, str): | ||
| name = gen_variable_name(item) |
There was a problem hiding this comment.
local variable 'name' is assigned to but never used
cartoframes/vis/popup.py
Outdated
| """ | ||
|
|
||
| def __init__(self, data=None): | ||
| self._init_popup(data) |
| attrs.forEach((item) => { | ||
| let value = feature.variables[item.name].value; | ||
| if (typeof value === 'number') { | ||
| if (Math.abs(value) > 1000) { |
There was a problem hiding this comment.
This is fine for now, but we should probably use a proper formatter for this
|
Tests added / updated. |
cartoframes/vis/layer.py
Outdated
| popup (dict, :py:class:`Popup <cartoframes.vis.Popup>`, optional): | ||
| This option adds interactivity (click and hover) to a layer to show popups. | ||
| The columns to be shown must be added in a list format for each event. It | ||
| must be written using `CARTL VL expressions syntax |
There was a problem hiding this comment.
typo: using CARTO VL expressions syntax
cartoframes/vis/popup.py
Outdated
| Args: | ||
| data (dict): The popup definition for a layer. It contains the information | ||
| to show a popup on 'click' and 'hover' events with the attributes provided | ||
| in the definition using the `VL expressions syntax |
There was a problem hiding this comment.
typo: using the CARTO VL expressions syntax (maybe is not a typo, but I wouldn't use only VL)
| raise ValueError('Wrong popup input') | ||
| return output | ||
|
|
||
| def get_variables(self): |
There was a problem hiding this comment.
As a suggestion, I think _get_vars method shouldn't modify the input since it's supposed, by its name, to return some variables. I would do something like:
def _parse_popup_variables(self, variables):
popup_variables = {}
# same but using o popup_variables instead of variables and variables instead of array
return popup_variablesdef get_variables(self):
return self._parse_vars(self._click + self._hover)Since the values would be overwritten we could use a Set ( return self._parse_vars(list(set(self._click + self._hover)))), but I think this is only available in Python 3.
There was a problem hiding this comment.
True. This is a nice refactor :)
There is not a problem with repeated variables because each key is related 1:1 with the value.
| 'v8e0f74': '$name' | ||
| }) | ||
|
|
||
| def test_wrong_attribute(self): |
Related to #677.