Skip to content
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

Support for wildcard attributes, implement data-* attribute #237

Merged
merged 5 commits into from Apr 23, 2018

Conversation

rmarren1
Copy link
Contributor

Adds the data-* html attributes, like so:

>>> import dash_html_components as html
>>> html.Div(id="my-id", **{'data-foo': 'foo', 'data-bar': 'bar'})
Div(id='my-id', data-foo='foo', data-bar='bar')

Tested using Tox, however I

  1. removed the python -m unittest tests.test_react test, since this is broken in Tox tests fail on master branch #236 and Events are broken; base_component.parse_events needs updating #232
  2. added the python -m unittest tests.development.test_base_component and python -m unittest tests.development.test_component_loader tests, since only running tox was not causing these tests to run (or are are these tests generally invoked in another way?)

List of Dash valid wildcard prefixes
"""
list_of_valid_wildcard_attr_prefixes = []
for wildcard_attr in ["data-*"]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose "aria-*" too

@chriddyp
Copy link
Member

chriddyp commented Apr 3, 2018

This looks great, thanks @rmarren1 !

app = dash.Dash()
app.layout = html.Div([
    html.Div(
        id='data-element',
        **{
            'data-string': 'multiple words',
            'data-number': 512,
            'data-none': None,
            'data-date': datetime.datetime(2012, 1, 10),
            'aria-progress': 5
        }
    )
])

self.startServer(app)

div = self.wait_for_element_by_id('data-element')
self.assertEqual(
    div.get_attribute('innerHTML'),
    (
        '<div id="data-element" '
        'data-string="multiple words" '
        'data-number="512" '
        'data-none=""'
        'data-date="2012-01-10"
        'aria-progress="5"'></div>'
)
  • Similarly, let's add an integration test that updates these attributes

I'll take a look at plotly/dash-renderer#51 now


  • Edit: Added aria-progress to the test example

@chriddyp
Copy link
Member

chriddyp commented Apr 3, 2018

I've published a prerelease version at pip install dash==0.22.0rc1

@chriddyp
Copy link
Member

chriddyp commented Apr 3, 2018

Added some comments in plotly/dash-renderer#51 (comment) that actually apply this repo

@rmarren1
Copy link
Contributor Author

rmarren1 commented Apr 4, 2018

**{
'data-string': 'multiple words',
'data-number': 512,
'data-none': None,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value will not be rendered in the html div.

@rmarren1
Copy link
Contributor Author

rmarren1 commented Apr 4, 2018

Test is dependent on changes in plotly/dash-html-components#40

@claresloggett
Copy link

claresloggett commented Apr 7, 2018

This is awesome, thank you!

I see that to implement this, you're now checking not just for valid attributes but also for attributes that match valid wildcards, i.e. 'data-' and 'aria-'. A question - and I'm totally open to being convinced this is a bad idea - is there a reason to not simply allow any attributes, i.e. just not bother checking them? From my naive point of view that seems more future-proof. Could it be harmful somehow?

Edit: thanks for the explanation @chriddyp !

@chriddyp
Copy link
Member

chriddyp commented Apr 7, 2018

is there a reason to not simply allow any attributes, i.e. just not bother checking them

Yeah, the main issue is that higher-level React based components (i.e. components that don't just render the HTML components) might not actually end up rendering these attributes.

For example, here is the dcc.Slider component: https://github.com/plotly/dash-core-components/blob/d034e25f6f56423ef2de7e5905328396040cf4d8/src/components/Slider.react.js#L23. It is just a light wrapper around the 3rd party rc-slider component (https://react-component.github.io/slider/examples/slider.html) which might not actually end up rendering data-* attributes in the DOM.

So, if we don't do validation of properties on the component-level, then the end user might think that their data-* attributes are getting rendered in the DOM when they actually aren't. It's up to the component author to make sure that their component will render these attributes.

Re future proof - once the component author ends up supporting them, the end users will be able to upgrade their package to the new version that supports them. If they don't get errors on previous versions, then the end user might not be aware that they are working on a version that doesn't yet support these properties and they won't be aware that they need to upgrade.

# Add the wildcard properties data-* and aria-*
props.update({
k: getattr(self, k)
for k in self.__dict__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, way better 👍

'id': 'a',
},
'type': 'MyComponent'}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


input_call_count = Value('i', 0)

@app.callback(Output('output-1', 'data-cb'), [Input('input', 'value')])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very cool that this works

@chriddyp
Copy link
Member

chriddyp commented Apr 9, 2018

image

Very, very nice @rmarren1 🎉

  • I've published a prerelease version of dash-html-components at dash-html-components==0.11.0rc1. Let's update the dev-requirements.txt file with this new version so that we can make sure that circleci passes the tests. After everything is merged and released, we can move it up to the new stable version at 0.11.0.

  • Finally, let's update the CHANGELOG.md and version.py file. That way, once tests pass I can merge and release immediately.

@rmarren1
Copy link
Contributor Author

This is down by the way" https://unpkg.com/dash-html-components@0.11.0rc1/dash_html_components/bundle.js

@chriddyp
Copy link
Member

@rmarren1 - Thank you! This is really great stuff. I'm 👍 with these changes. Is there anything else that you would like to add? Otherwise, I will merge and release.

@rmarren1
Copy link
Contributor Author

rmarren1 commented Apr 11, 2018

I'm good with it, go ahead!

@kmader
Copy link

kmader commented Apr 16, 2018

@chriddyp to get it working I needed to do both
pip install dash==0.22.0rc1 and
pip install dash_html_components==0.11.0rc1

@rmarren1
Copy link
Contributor Author

I needed dash==0.22.0rc2

@chriddyp chriddyp merged commit 3dfa941 into plotly:master Apr 23, 2018
@claresloggett
Copy link

claresloggett commented May 6, 2018

I'm using dash==0.22.0rc2 and dash_html_components==0.11.0rc1, but as @rmarren1 points out https://unpkg.com/dash-html-components@0.11.0rc1/dash_html_components/bundle.js appears to be down.

So far as I can tell the fix is to use app.scripts.config.serve_locally=True, but this makes it hard for me to load other external JavaScript files (I am trying to also load Bootstrap). Can I currently get this PR working without serve_locally=True? @chriddyp can you say whether the hosting of https://unpkg.com/dash-html-components@0.11.0rc1/ will go back up in the near future? dash==0.22.0rc2 seems to be fine.

@ned2
Copy link
Contributor

ned2 commented May 6, 2018

@claresloggett, one workaround for using serve_locally=True while also using your own custom JS scripts is to subclass the Dash instance and add the scripts directly into the HTML head tag. You can see an example of this but used for adding custom CSS scripts here.

@rmarren1
Copy link
Contributor Author

rmarren1 commented May 6, 2018

Try dash-html-components==0.11.0rc5 dash==0.21.1

@claresloggett
Copy link

@rmarren1 thank you, those versions are working for me! \o/

@ned2 that's good advice thanks, it's a workaround for other issues too. I had seen it, just thought I'd check if I really needed to go down that route. For others' reference there is also an open PR to support more flexible JS and CSS inclusion: #171

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants