[BUG] Dropdown className is not passed to HTML

orenbenkiki commented 5 years ago

pip list

dash (1.0.2)
dash-bootstrap-components (0.7.0)
dash-core-components (1.0.0)
dash-daq (0.1.7)
dash-html-components (1.0.0)
dash-renderer (1.0.0)
dash-table (4.0.2)

Describe the bug

Writing dcc.Dropdown(id='myDropdown', className='myClass', ...) does not work.

Expected behavior

The className should be passed to the generated HTML, specifically to the div with the specified id=myDropdown.

Actual behavior

The className does not appear anywhere in the generated HTML.

Example

import dash
import dash_core_components as dcc
import dash_html_components as html

app = dash.Dash()
app.layout = dcc.Dropdown(id='myDropdown', className='myClass')
app.run_server(debug=True)

Screenshots

byronz commented 5 years ago

@orenbenkiki the className is present but in a parent wrapper div.

orenbenkiki commented 5 years ago

@orenbenkiki the className is present but in a parent wrapper div.

Ugh, missed that. This is still a bug, because:

Technically, the class is applied to a child div. One would expect the class to be applied to the div which gets the id, that is, the outermost (parent-most) div.
Aside from consistency, this also has a good CSS reason. Specifically, a CSS rule can't select the parent of a div with a given class - but it can select the child (or grand-child) of a div with a given class. So if marking the outer-most div with the class, one can write .myClass > .Select to pick the Select div. In the current way of marking the inner div with the class, there is no CSS rule that will select the outer-most div - There is no .myClass < .Select selector rule in CSS.
Some CSS styles must be applied to the outer-most div. Specifically, specifying width using the flex model requires that the div will be a direct child of the flex container. Therefore, specifying .myClass { flex-grow: 1; } does not currently work, but would have worked if the class was applied to the outer-most div.

I therefore suggest that the class should be moved to the outer-most div instead of being applied to the inner Select div.

Alternatively, the two div elements should be merged to a single div. I'm not certain why they are separate, the only content I see in the outermost div is the inner div. If merging the two is acceptable, I think this would maximize compatibility with existing CSS rules (e.g., if someone wrote some CSS styles for Select that must be applied to the direct parent of the Select-control).

byronz commented 5 years ago

Technically, the class is applied to a child div. One would expect the class to be applied to the div which gets the id, that is, the outermost (parent-most) div.

@orenbenkiki agreed, this inconsistency is not only in dropdown, upload has the same issue.

Alternatively, the two div elements should be merged to a single div. I'm not certain why they are separate, the only content I see in the outermost div is the inner div. If merging the two is acceptable, I think this would maximize compatibility with existing CSS rules (e.g., if someone wrote some CSS styles for Select that must be applied to the direct parent of the Select-control).

the inner div is from the wrapped react component and other components have a similar approach. we will probably improve that with a more elegant approach with the bump up of react 17, which needs some fundamental refactoring in the DCC source code.

https://github.com/plotly/dash-core-components/blob/5ed92d7a506101ccd84cc19028fc10ad8e3a3aa0/src/components/Dropdown.react.js#L73-L80

byronz commented 5 years ago

@wbrgss do you foresee any impact on DDK if we move the className for dropdown and upload?

orenbenkiki commented 5 years ago

Looking at https://github.com/plotly/dash-core-components/blob/5ed92d7a506101ccd84cc19028fc10ad8e3a3aa0/src/components/Dropdown.react.js#L73-L80 - is it not possible to simply move the id=... style=... data-dash-is-loading=... into the ReactDropdown element, and get rid of the wrapper div? I would have expected it to be possible to attach attributes to the top-level HTML element in React...? That said, I've never (directly) used React myself, so there may be a good reason why this wouldn't work.

wbrgss commented 5 years ago

@byronz Yeah, I see an impact; the dropdown is one of the more heavily styled components. It will probably nullify some style rules and affect the specificity of others. I should be able to adapt DDK fairly easily though thanks to your heads up. Please keep me informed if/when there's an upcoming PR to address this.

I agree that there is some inconsistency in where these attributes are applied to the HTML elements. This is a very good point from @orenbenkiki, and is a problem sometimes in other Dash components:

Specifically, a CSS rule can't select the parent of a div with a given class - but it can select the child (or grand-child) of a div with a given class.

However, we should consider whether this is truly a bug, and consider to what extent it is a breaking change. It is quite likely that there are Dash apps that have custom stylesheets that rely on the hierarchy and placement of the className attribute, and moving it would technically introduce visual regressions into these apps.

One way around this breaking change is to moving the className further up in the DOM hierarchy to the parent, but appending something like --parent to it. So the structure would look like this:

<div id="myDropdown" class="myClass--parent">
    <div class="Select myClass ...">

This looks kind of ugly, implicitly mutates the custom className, and would have to be documented, but it's a safer compromise. Both elements could be targeted if need be, and any existing apps that don't need to target the parent div would continue to appear as expected.

the two div elements should be merged to a single div

This is the route I would take if we don't already have a Dash 1.0 out. Unless you want to put it in a 2.0 ;). What do you think @byronz? Am I being to strict about semantic versioning?

byronz commented 5 years ago

@wbrgss let's bring this on Friday's meeting

wbrgss commented 5 years ago

Update from the meeting, as per @alexcjohnson's idea we agreed we should have an extra class prop that can be optionally set on the parent e.g. parentClassName. We should try to be consistent with e.g. dcc.Tab attribute naming conventions, which include props like disabled_className, selected_className etc. Although, I think it's important to note that, as their names suggest, these classNames are added to the same element based on its state — not to different elements in the component based on their hierarchy

cc @byronz if you have anything to add

alexcjohnson commented 5 years ago

The parent_className prop of Tabs is essentially an exact match for what we talked about here - true, some of them are different states of the same element, but this one is a constant on a different element

https://github.com/plotly/dash-core-components/blob/5ed92d7a506101ccd84cc19028fc10ad8e3a3aa0/src/components/Tabs.react.js#L253

alexcjohnson commented 5 years ago

@wbrgss @byronz note I transferred this issue to dcc as well

jimbroze commented 4 years ago

Is there a rough estimate of when this might be implemented?

plotly / dash-core-components

[BUG] Dropdown className is not passed to HTML #606