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

Input error passing label prop on false #505

Closed
wants to merge 12 commits into from

Conversation

aojin
Copy link
Contributor

@aojin aojin commented Nov 17, 2021

@aojin aojin requested a review from chawes13 November 17, 2021 21:02
@aojin aojin changed the base branch from master to expanded-props-cell November 17, 2021 21:02
@aojin aojin changed the base branch from expanded-props-cell to master November 17, 2021 21:02
@aojin aojin closed this Nov 17, 2021
@aojin aojin reopened this Nov 17, 2021
@aojin aojin linked an issue Nov 17, 2021 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
Comment on lines 17 to 18
const DefaultCellComponent = ({ className, onClick, placeholder, value, ...rest }) => // eslint-disable-line
<td { ...{ className, onClick, ...filterInvalidDOMProps(rest) }}>{ isNil(value) ? placeholder : value }</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

I imagine you might have branched off of expanded-props-cell when you made this change? Please remove

return (
<span>
{
label !== false &&
<label htmlFor={ id || name } className={ className }>
<label htmlFor={ id || name } className={ className } {...filterInvalidDOMProps(rest)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this tied to a specific issue?

Copy link
Contributor

@chawes13 chawes13 Nov 18, 2021

Choose a reason for hiding this comment

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

Ah I see, looks like this is related to #506. Can you revert these changes from this PR?

Comment on lines 94 to 100
.add('with custom properties on cell component per column', () => (
<SortableTable data={tableData}>
<Column name="name" customCellClass="test" />
<Column name="age" data-cy="age"/>
<Column name="active" aria-label="Active" />
</SortableTable>
))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above re: keeping your PR changes isolated

return hasInputError({ touched, invalid })
? <span
id={ generateInputErrorId(name) }
className={ classnames('error-message', className) }
{ ...filterInvalidDOMProps(rest) }
{ ...filterInvalidDOMProps(!label ? omit(rest, 'label') : rest) }
Copy link
Contributor

Choose a reason for hiding this comment

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

You're already omitting label from rest on line 74, so you shouldn't need this?

@chawes13
Copy link
Contributor

@aojin Do you have bandwidth to make these changes or would it be better to reassign?

@chawes13 chawes13 changed the base branch from main to v6 June 15, 2022 21:34
@chawes13
Copy link
Contributor

Turns out this was actually fixed in #380. Since this task is now getting included in the v7 milestone, I'm going to mark this as closed.

@chawes13 chawes13 closed this Jun 15, 2022
@chawes13 chawes13 deleted the input-error-passing-label-prop-on-false branch June 15, 2022 21:42
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.

Omit label props before passing them to default InputError component
2 participants