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

Update default props #2926

Merged
merged 13 commits into from
Nov 4, 2024
Merged

Update default props #2926

merged 13 commits into from
Nov 4, 2024

Conversation

AnnMarieW
Copy link
Collaborator

@AnnMarieW AnnMarieW commented Jul 22, 2024

closes #2919

I updated all the components listed in #2919 except for dcc.Location because it's still a class component.

  • validate if it works with the typescript components
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@AnnMarieW AnnMarieW marked this pull request as draft July 22, 2024 14:27
@gvwilson gvwilson added the infrastructure build process etc. label Jul 23, 2024
@gvwilson gvwilson assigned AnnMarieW and unassigned T4rk1n Jul 23, 2024
@AnnMarieW
Copy link
Collaborator Author

AnnMarieW commented Jul 23, 2024

Another dependency to update - our version of eslint does not seem to support the spread operator. eslint/eslint#10307

> eslint src scripts

/home/circleci/dash/components/dash-html-components/src/components/A.react.js
  12:52  error  Parsing error: Unexpected token ..

For example, this error is from defining the defaults like this in A.react.js

const A = ({n_clicks = 0, n_clicks_timestamp = -1, ...props}) => {

There is an error for each component.

@gvwilson gvwilson added feature something new P2 considered for next cycle and removed infrastructure build process etc. labels Aug 13, 2024
@gvwilson
Copy link
Contributor

@AnnMarieW can you please let us know if you're still working on this one or is it ready for review? thanks - @gvwilson

@AnnMarieW
Copy link
Collaborator Author

Hi @gvwilson

I'm afraid, I've hit a roadblock on this one. Here's the current status:

  • The components listed in issue 2919 are updated so that the defaults are defined in the function signature. I verified that the default props are set correctly in the component and the docstrings are generated correctly to show the defaults.
  • The sample typescript component is updated. I verified that the defaults are set correctly in the component, however the docstrings do not show the default values.

I found this project that parses the default props from the function signature in typescript components, however, I'm stuck trying to integrate that logic into extract-meta.js

If someone could help with that piece, I could probably finish the rest (ie tests) However, I'm traveling for a few weeks, and would be happy to turn this project over to someone if you'd like to get it wrapped up sooner.

@gvwilson
Copy link
Contributor

@T4rk1n please have a look at this one and help @AnnMarieW move it forward when you can. thanks - @gvwilson

@T4rk1n T4rk1n marked this pull request as ready for review October 16, 2024 20:11
Copy link
Contributor

@gvwilson gvwilson left a comment

Choose a reason for hiding this comment

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

some questions but no blockers

@@ -32,7 +32,7 @@ export type TypescriptComponentProps = {
array_elements?: JSX.Element[];

string_default?: string;
number_default?: string;
number_default?: number;
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems kind of important :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I changed this to get tests to pass, but @T4rk1n can you confirm if this is OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that is the good type.

@@ -43,7 +44,7 @@
"import/named": ["off"],
"import/namespace": ["off"],
"import/no-duplicates": ["error"],
"import/no-named-as-default": ["error"],
"import/no-named-as-default": ["off"],
Copy link
Contributor

Choose a reason for hiding this comment

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

apologies, but can you please explain what the impact of this one will be?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a linting that conflicted with the default notation on param functions.

display,
color,
display = 'auto',
color = '#119DFF',
Copy link
Contributor

Choose a reason for hiding this comment

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

do we have colors and other constants scattered elsewhere through the code as well or is there a way to consolidate them all in one place? (very low priority, not worth holding up this PR for)

show = true,
targetable = false,
direction = 'right',
border_color = '#d6d6d6',
Copy link
Contributor

Choose a reason for hiding this comment

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

again, can we (in future?) replace magic color constants with names that are all defined in one place?

@T4rk1n T4rk1n merged commit fa7d30a into plotly:dev Nov 4, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React 18 warning defaultProps will be removed from function components
3 participants