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

Fix prop types #523

Merged
merged 49 commits into from Apr 20, 2019

Conversation

Projects
None yet
3 participants
@chriddyp
Copy link
Member

commented Apr 12, 2019

in preparation for plotly/dash-renderer#100

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Show resolved Hide resolved src/components/Input.react.js Outdated
Show resolved Hide resolved src/components/Input.react.js Outdated
@byronz

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

@chriddyp To keep DRY 🐫 we might need a similar mechanism to share some of the global attributes definition like in HTML components.

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

fixes #457
fixes #505

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

To keep DRY 🐫 we might need a similar mechanism to share some of the global attributes definition like in HTML components.

Yeah I agree. I'd like to punt this one to the future. For now, just want to make sure that we won't be breaking any valid code.

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2019

Alright, I've addressed your comments @alexcjohnson @byronz

- `inputmode` is now `inputMode`
- `maxlength` is now `maxLength`
- `minlength` is now `minLength`
- Updated property definitions of various components in anticipation of upcoming component validation (https://github.com/plotly/dash-renderer/pull/100/). [#523](https://github.com/plotly/dash-core-components/pull/523/)

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 13, 2019

Contributor

updated, or fixed? Perhaps "improved" since it's a little of each I guess.

This comment has been minimized.

Copy link
@byronz

byronz Apr 15, 2019

Contributor

👍 for improved, or we could have two categories in release log: 1 bug fix 2 feature enhancement

@@ -62,7 +62,7 @@ export default class Input extends Component {
if (setProps) {
const payload = {
n_blur: this.props.n_blur + 1,
n_blur_timestamp: new Date(),
n_blur_timestamp: new Date.now(),

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 13, 2019

Contributor

This is a breaking change #467 - yes, we should do it, but we should call it out.

debounce: PropTypes.oneOfType([
PropTypes.string,
PropTypes.bool
]),

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 13, 2019

Contributor

What's string for here? Describe it in the docstring?

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 13, 2019

Contributor

^^ applies to a bunch of these where either string or bool now accepts either... it's not clear to me which strings do what, or if they all evaluate to booleans or do something else entirely.

This comment has been minimized.

Copy link
@chriddyp

chriddyp Apr 14, 2019

Author Member

this one was wrong actually - only bool. But for the other HTML boolean properties, HTML5 defines the only accepted values as the name of the attribute (case insensitive) or "": https://html.spec.whatwg.org/multipage/common-microsyntaxes.html#boolean-attributes

That is, for autoFocus, the following assignments mean "turn on autofocus": autoFocus="", autoFocus="autoFocus", autoFocus="autofocus", autoFocus="AUTOFOCUS"

And in order for it to be "turned off", it has to be omitted.

It seems like browsers are a little more loose and they evaluate these strings to be "If it's supplied, then it's applied" - it doesn't need to be the name of the attribute. This ends up being confusing because checked="false" is actually "turn on checked" because it's applied. MDN says:

To be clear, the values "true" and "false" are not allowed on boolean attributes. To represent a false value, the attribute has to be omitted altogether. This restriction clears up some common misunderstandings: With checked="false" for example, the element’s checked attribute would be interpreted as true because the attribute is present.

So, we could either:

  1. Just keep it as a string and allow users to navigate this spec. Move on for now and revist later.
  2. Provide a preference but allow all strings: autoFocus: oneOfType([oneOf(['autoFocus', 'autofocus', '']), propTypes.string])
  3. Be more opionated and only include the common forms like 'autoFocus': oneOf(['', 'autoFocus', 'autofocus', 'AUTOFOCUS']) but omit the other permutations like auToFoCus that are technically valid.
  4. Be even more opinionated and only include the most sane-looking version: autoFocus='autoFocus' and omit the other versions that are technically valid.

This comment has been minimized.

Copy link
@chriddyp

chriddyp Apr 14, 2019

Author Member

I think I prefer 2 or 3

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 14, 2019

Contributor

So does false (not 'false') turn autoFocus off? Or is None -> null the only value that disables it?

If this check is only applied in dev mode, I'd vote to be more opinionated - (3) seems reasonable, plus false if that's allowed to disable. I'd omit '' unless it's particularly common usage, as explicitly allowing a normally falsy value to mean true is bound to cause confusion. Either way we should document a recommended value, something like:

autoFocus is enabled by 'autoFocus', but alternate capitalizations 'autofocus' and 'AUTOFOCUS' are also accepted.

This comment has been minimized.

Copy link
@chriddyp

chriddyp Apr 14, 2019

Author Member

I thought that false ended up getting rendered as 'false' looks like I'm wrong about that:
image

(https://codepen.io/chriddyp/pen/MREmpK?editors=0010)

Similarly in our toolchain:

import dash
import dash_html_components as html
import dash_core_components as dcc

app = dash.Dash(__name__)
app.layout = html.Div([
    dcc.Input(id='1', readOnly=True),
    dcc.Input(id='2', readOnly=False),
    dcc.Input(id='3', readOnly='readonly'),
    dcc.Input(id='4', readOnly='readOnly'),
    dcc.Input(id='5', readOnly='true'),
    dcc.Input(id='6', readOnly='false'),
    dcc.Input(id='7', readOnly=''),
    dcc.Input(id='8', readOnly='""'),
])

if __name__ == '__main__':
    app.run_server(debug=True)

image

#7 is pretty confusing - to get "" into the DOM, you have to pass in '""' not ''

So I suppose we could allow boolean for these properties as well.

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Apr 14, 2019

Contributor

Oy, 7 is insane, definitely do not allow any version of that! So yeah, true, false, and a couple of capitalizations of autoFocus.

This comment has been minimized.

Copy link
@byronz

byronz Apr 15, 2019

Contributor

a small section about the attributes in dash-docs might be necessary given all the experiments we have, as most of our audiences are not Frontend expert, we should align the preference in the demo code too

@@ -27,7 +27,7 @@ jobs:
name: Install dependencies (dash)
command: |
git clone git@github.com:plotly/dash.git
git clone git@github.com:plotly/dash-renderer.git
git clone --branch chriddyp-dev-tools git@github.com:plotly/dash-renderer.git

This comment has been minimized.

Copy link
@chriddyp

chriddyp Apr 15, 2019

Author Member

I'll remove this before merging. Just making sure that the other repo's prop checking doesn't cause these tests to fail.

@@ -162,19 +162,30 @@ Input.propTypes = {
/**
* This attribute indicates whether the value of the control can be automatically completed by the browser.
*/
autocomplete: PropTypes.string,
autoComplete: PropTypes.string,

This comment has been minimized.

Copy link
@byronz

byronz Apr 16, 2019

Contributor

yes, given the complexity of autoComplete, I think string is a good compromise for now

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Alright, this is passing with the dev-tools branch now 🎉 .

  • Had to make a fix to ConfirmModal so that it was idempotent, see 8b64f2d
  • .exact didn't have a proper docstring. This docstring was pretty nice for the long list of properties in Graph.config, so I added support for exact in the component generation in plotly/dash#692
  • .exact also wasn't supported in the current version of react-docgen - upgrading to 3.0.0 fixed the issue.
@byronz

byronz approved these changes Apr 18, 2019

Copy link
Contributor

left a comment

💃

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

weird dateformat diff. 05/03/1997 vs 1997-05-03
image

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

that's the only error left

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Odd - the new screenshots match the date supplied exactly. We send dt_input_1.send_keys("1997-05-03") and it appears exactly as "1997-05-03" instead of getting reformatted as 05/03/1997.

Odd that this behaviour would change in this PR. Looking into it...

chriddyp added some commits Apr 12, 2019

html property handling
- boolean props - can be bools or strings, right?
- number props - can be numbers or stringified numbers, right?
marks prop type was wrong
no way to specify that the values of an object need to be a certain
type if the object has arbitrary key names.
date number not date object
this matches the other date properties most notably `n_clicks_timestamp`

byronz and others added some commits Apr 18, 2019

@chriddyp chriddyp force-pushed the fix-prop-types branch from 3c4409a to 41a3f31 Apr 18, 2019

@chriddyp chriddyp referenced this pull request Apr 18, 2019

Closed

WIP - R dbg #531

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

OK - I've isolated the issue. There seems to be a problem with dash-renderer's setProps not re-rendering the component with the correct prop types if an ID is not supplied. I'll look into dash-renderer next.

@chriddyp

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

Here's a reproducable example that shows the issue (try editing the dates). I'll work on a fix in dash-renderer now.

from datetime import datetime as dt
import dash
import dash_html_components as html
import dash_core_components as dcc

app = dash.Dash(__name__)

app.css.config.serve_locally = True
app.scripts.config.serve_locally = True

app.layout = html.Div([
    html.Div([
        html.Label('With ID'),
        dcc.DatePickerSingle(id='date-picker-with-id'),
    ]),

    html.Div([
        html.Label('Without ID'),
        dcc.DatePickerSingle(),
    ])

])


if __name__ == '__main__':
    app.run_server(debug=True, dev_tools_hot_reload=False)

@byronz byronz merged commit fecc76e into master Apr 20, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details

@byronz byronz deleted the fix-prop-types branch Apr 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.