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: Use JSX.ImplicitElements to derive valid property names #267

Merged
merged 2 commits into from
Jun 30, 2020

Conversation

ahobson
Copy link
Contributor

@ahobson ahobson commented Jun 18, 2020

Summary

Use the proper react types to indicate which props are allowed on components.

Related Issues or PRs

This was found as part of exploration for #161

How To Test

This should not change functionality other than allowing the proper attributes

@ahobson ahobson added the status: wip Work in progress - not ready for code review label Jun 18, 2020
@ahobson ahobson requested review from suzubara and haworku June 18, 2020 19:38
@ahobson ahobson force-pushed the adh-propredo branch 4 times, most recently from ac5445e to 00736d5 Compare June 24, 2020 16:51
@haworku haworku changed the title WIP: Use JSX.ImplicitElements to derive valid property names [WIP] Use JSX.ImplicitElements to derive valid property names Jun 24, 2020
@@ -19,7 +19,7 @@ export const MegaMenu = (
<div className="grid-row grid-gap-4">
{items.map((listItems, i) => (
<div className="usa-col" key={`subnav_col_${i}`}>
<NavList items={listItems} type="megamenu" {...ulProps} />
<NavList items={listItems} megamenu={true} {...navListProps} />
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: for megamenu={true} and I think I saw this one other place. Leave off the attribute assignment when its true. Just by including the name of the attribute on the element it is by default true.

data-testid="tag"
className={tagClasses}
style={{ ...style }}
{...spanProps}>
Copy link
Contributor

@haworku haworku Jun 24, 2020

Choose a reason for hiding this comment

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

Thanks for catching stuff like this, I think I saw a couple other places where this rewrite was also nice little cleanup for us. Also like how you standardized the way we name props -CustomProps etc.

Copy link
Contributor

@haworku haworku left a comment

Choose a reason for hiding this comment

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

This seems to work! I kept trying JSX.IntrinsicElements.form earlier and that was not working haha didn't even try bracket notation. Would like @suzubara to also take a look before we merge.

Also random FYI when you use [WIP] the PR title linter will pass.

Copy link
Contributor

@suzubara suzubara left a comment

Choose a reason for hiding this comment

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

Looks good to me! Just need to take [WIP] out of the PR title and make it match the semantic changelog format.

@ahobson
Copy link
Contributor Author

ahobson commented Jun 26, 2020

Looks good to me! Just need to take [WIP] out of the PR title and make it match the semantic changelog format.

Ok! Should I create a corresponding issue about it? Is this a bugfix or a feature?

@haworku
Copy link
Contributor

haworku commented Jun 26, 2020

@ahobson I think fix is fine with the same PR title you have. Also don't feel a need to make an issue since this came up in #161 somewhat organically.

@haworku haworku removed the status: wip Work in progress - not ready for code review label Jun 29, 2020
@ahobson ahobson changed the title [WIP] Use JSX.ImplicitElements to derive valid property names fix: Use JSX.ImplicitElements to derive valid property names Jun 29, 2020
@ahobson ahobson merged commit c5ff866 into develop Jun 30, 2020
@ahobson ahobson deleted the adh-propredo branch June 30, 2020 18:22
haworku pushed a commit that referenced this pull request Jul 10, 2020
* Use JSX.ImplicitElements to derive valid property names

* Use name of attribute only when value is true
haworku pushed a commit that referenced this pull request Jul 13, 2020
* Use JSX.ImplicitElements to derive valid property names

* Use name of attribute only when value is true
@haworku haworku mentioned this pull request Jul 13, 2020
haworku pushed a commit that referenced this pull request Jul 13, 2020
* Use JSX.ImplicitElements to derive valid property names

* Use name of attribute only when value is true
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.

3 participants