Skip to content

Commit

Permalink
First chunk of PR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
petar-qb committed Nov 26, 2024
1 parent ca6b4ad commit 9fd0f1d
Show file tree
Hide file tree
Showing 8 changed files with 15 additions and 41 deletions.
2 changes: 1 addition & 1 deletion vizro-core/examples/scratch_dev/data.yaml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Choose between 0-50
setosa: 5
#versicolor: 10
versicolor: 10
virginica: 15

# Choose between: 4.3 to 7.4
Expand Down
1 change: 0 additions & 1 deletion vizro-core/src/vizro/actions/_actions_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,6 @@ def _get_modified_page_figures(
**_get_parametrized_config(ctds_parameter=ctds_parameter, target=target, data_frame=False),
)

# AM comment: please check this comment I added!
for target in control_targets:
ctd_filter = [item for item in ctds_filter if item["id"] == model_manager[target].selector.id]

Expand Down
9 changes: 2 additions & 7 deletions vizro-core/src/vizro/models/_components/form/checklist.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ class Checklist(VizroBaseModel):
_validate_value = validator("value", allow_reuse=True, always=True)(validate_value)

def __call__(self, options):
return self._build_static(options)

def _build_static(self, options):
full_options, default_value = get_options_and_default(options=options, multi=True)

return html.Fieldset(
Expand All @@ -71,10 +68,8 @@ def _build_dynamic_placeholder(self):
if self.value is None:
self.value = [get_options_and_default(self.options, multi=True)[1]]

return self._build_static(self.options)
return self.__call__(self.options)

@_log_call
def build(self):
# We don't have to implement _build_dynamic_placeholder, _build_static here. It's possible to: if dynamic and
# self.value is None -> set self.value + return standard build (static), but let's align it with the Dropdown.
return self._build_dynamic_placeholder() if self._dynamic else self._build_static(self.options)
return self._build_dynamic_placeholder() if self._dynamic else self.__call__(self.options)
14 changes: 5 additions & 9 deletions vizro-core/src/vizro/models/_components/form/dropdown.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,7 @@ def validate_multi(cls, multi, values):
raise ValueError("Please set multi=True if providing a list of default values.")
return multi

# AM comment: please remove build_static and change into __call__ in all places unless you think there's
# a good reason not to do so.
def __call__(self, options):
# AM comment: this is the main confusing thing about the current approach I think: every time we run
# __call__ we override the value set below, which sounds wrong because we say that we never change the value.
# From what I remember this is a workaround for the Dash bug? Maybe add a comment explaining this.
full_options, default_value = get_options_and_default(options=options, multi=self.multi)
option_height = _calculate_option_height(full_options)

Expand Down Expand Up @@ -124,11 +119,12 @@ def _build_dynamic_placeholder(self):
# TODO-NEXT: Replace this with the "universal Vizro placeholder" component.
return html.Div(
children=[
dbc.Label(self.title, html_for=self.id) if self.title else None,
# AM question: why do we want opacity: 0? If we don't want it to appear on screen then normally we do
# this with visibility or display.
dmc.DateRangePicker(
id=self.id, value=self.value, persistence=True, persistence_type="session", style={"opacity": 0}
id=self.id,
value=self.value,
persistence=True,
persistence_type="session",
style={"visibility": "hidden"},
),
]
)
Expand Down
9 changes: 2 additions & 7 deletions vizro-core/src/vizro/models/_components/form/radio_items.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,6 @@ class RadioItems(VizroBaseModel):
_validate_value = validator("value", allow_reuse=True, always=True)(validate_value)

def __call__(self, options):
return self._build_static(options)

def _build_static(self, options):
full_options, default_value = get_options_and_default(options=options, multi=False)

return html.Fieldset(
Expand All @@ -72,10 +69,8 @@ def _build_dynamic_placeholder(self):
if self.value is None:
self.value = get_options_and_default(self.options, multi=False)[1]

return self._build_static(self.options)
return self.__call__(self.options)

@_log_call
def build(self):
# We don't have to implement _build_dynamic_placeholder, _build_static here. It's possible to: if dynamic and
# self.value is None -> set self.value + return standard build (static), but let's align it with the Dropdown.
return self._build_dynamic_placeholder() if self._dynamic else self._build_static(self.options)
return self._build_dynamic_placeholder() if self._dynamic else self.__call__(self.options)
10 changes: 2 additions & 8 deletions vizro-core/src/vizro/models/_components/form/range_slider.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,6 @@ class RangeSlider(VizroBaseModel):
_set_actions = _action_validator_factory("value")

def __call__(self, min, max, current_value):
return self._build_static(min, max, current_value)

@_log_call
def _build_static(self, min, max, current_value):
output = [
Output(f"{self.id}_start_value", "value"),
Output(f"{self.id}_end_value", "value"),
Expand Down Expand Up @@ -142,15 +138,13 @@ def _build_static(self, min, max, current_value):
)

def _build_dynamic_placeholder(self, current_value):
return self._build_static(self.min, self.max, current_value)
return self.__call__(self.min, self.max, current_value)

@_log_call
def build(self):
# We don't have to implement _build_dynamic_placeholder, _build_static here. It's possible to: if dynamic and
# self.value is None -> set self.value + return standard build (static), but let's align it with the Dropdown.
current_value = self.value or [self.min, self.max] # type: ignore[list-item]
return (
self._build_dynamic_placeholder(current_value)
if self._dynamic
else self._build_static(self.min, self.max, current_value)
else self.__call__(self.min, self.max, current_value)
)
9 changes: 2 additions & 7 deletions vizro-core/src/vizro/models/_components/form/slider.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ class Slider(VizroBaseModel):
_set_actions = _action_validator_factory("value")

def __call__(self, min, max, current_value):
return self._build_static(min, max, current_value)

def _build_static(self, min, max, current_value):
output = [
Output(f"{self.id}_end_value", "value"),
Output(self.id, "value"),
Expand Down Expand Up @@ -125,15 +122,13 @@ def _build_static(self, min, max, current_value):
)

def _build_dynamic_placeholder(self, current_value):
return self._build_static(self.min, self.max, current_value)
return self.__call__(self.min, self.max, current_value)

@_log_call
def build(self):
# We don't have to implement _build_dynamic_placeholder, _build_static here. It's possible to: if dynamic and
# self.value is None -> set self.value + return standard build (static), but let's align it with the Dropdown.
current_value = self.value if self.value is not None else self.min
return (
self._build_dynamic_placeholder(current_value)
if self._dynamic
else self._build_static(self.min, self.max, current_value)
else self.__call__(self.min, self.max, current_value)
)
2 changes: 1 addition & 1 deletion vizro-core/src/vizro/models/_controls/filter.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ def pre_build(self):
def build(self):
selector_build_obj = self.selector.build()
# TODO: Align the (dynamic) object's return structure with the figure's components when the Dash bug is fixed.
# This means returning an empty "html.Div(id=self.id, className="...")" as a placeholder from Filter.build().
# This means returning an empty "html.Div(id=self.id, className=...)" as a placeholder from Filter.build().
# Also, make selector.title visible when the filter is reloading.
return dcc.Loading(id=self.id, children=selector_build_obj) if self._dynamic else selector_build_obj

Expand Down

0 comments on commit 9fd0f1d

Please sign in to comment.