Skip to content

Conversation

@boaz0
Copy link
Member

@boaz0 boaz0 commented Jan 23, 2020

What: Closes #3549

//cc @jeff-phillips-18

@patternfly-build
Copy link
Collaborator

PatternFly-React preview: https://patternfly-react-pr-3555.surge.sh

@codecov-io
Copy link

codecov-io commented Jan 23, 2020

Codecov Report

Merging #3555 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3555   +/-   ##
=======================================
  Coverage   67.11%   67.11%           
=======================================
  Files         903      903           
  Lines       25502    25502           
  Branches     2262     2262           
=======================================
  Hits        17115    17115           
  Misses       7346     7346           
  Partials     1041     1041
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.29% <ø> (ø) ⬆️
#patternfly4 64.21% <ø> (ø) ⬆️
Impacted Files Coverage Δ
...e/src/components/Dropdown/InternalDropdownItem.tsx 64.89% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 327402f...128475e. Read the comment docs.

@jeff-phillips-18
Copy link
Member

Could we simply not set the id if not given? So either remove the default props for id and componentId or, if linter doesn't allow it, then do:

id={id || undefined}

@boaz0
Copy link
Member Author

boaz0 commented Jan 23, 2020

@jeff-phillips-18 - yes, that makes more sense. Thanks for the feedback.

Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

id: '',
componentID: '',
id: null,
componentID: null,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting this to null, couldn't we just not set a default? That is, remove this from defaultProps.

We shouldn't need to test componentID || undefined because it would already be undefined.

Copy link
Member

Choose a reason for hiding this comment

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

that was my thought as well, but if i recall the lint settings require optional props to have defaults, though undefined may be a valid default 🤷‍♂

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah if you remove the default props here then it will just default to undefined and be fine.

Copy link
Contributor

@dlabaj dlabaj left a comment

Choose a reason for hiding this comment

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

Can you update this to no set a default for id and componentID, then you won't need the checks like dan suggested. Also title seems wrong for this PR seems. Thanks.

Signed-off-by: Boaz Shuster <boaz.shuster.github@gmail.com>
Copy link
Member

@dlabrecq dlabrecq left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jeff-phillips-18 jeff-phillips-18 left a comment

Choose a reason for hiding this comment

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

👍 Glad we got rid of the default value lint checks!

@dlabaj dlabaj merged commit 06df46a into patternfly:master Jan 27, 2020
@boaz0 boaz0 deleted the closes_3549 branch April 24, 2020 06:41
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.

DropdownItems have bad id, cause console warnings

6 participants