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

Addon-info: allow duplicate displayNames #2269

Merged
merged 4 commits into from
Nov 9, 2017

Conversation

Hypnosphi
Copy link
Member

Issue: #1814
When using HOCs, different component types may have the same names. #1607 fixed it for name property, but duplicate diplayNames still lead to "same key" warnings.

What I did

Added indices to propTable div keys

How to test

yarn start & open http://localhost:9010/?selectedKind=GitHub%20issues&selectedStory=%231814

</h2>
const propTables = array.map((type, i) => (
// eslint-disable-next-line react/no-array-index-key
<div key={`${getName(type)}_${i}`}>
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's OK to use indices in this case

@codecov
Copy link

codecov bot commented Nov 8, 2017

Codecov Report

Merging #2269 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2269      +/-   ##
==========================================
+ Coverage   22.13%   22.14%   +0.01%     
==========================================
  Files         268      268              
  Lines        5892     5893       +1     
  Branches      715      712       -3     
==========================================
+ Hits         1304     1305       +1     
- Misses       4044     4052       +8     
+ Partials      544      536       -8
Impacted Files Coverage Δ
addons/info/src/components/Story.js 76.47% <100%> (+0.28%) ⬆️
lib/ui/src/modules/ui/containers/layout.js 12.5% <0%> (ø) ⬆️
...rc/modules/ui/components/left_panel/text_filter.js 30.98% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 21.36% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
lib/codemod/src/transforms/update-addon-info.js 50% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 48.8% <0%> (ø) ⬆️
... and 21 more

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 b078de7...aae332b. Read the comment docs.


const TextArea = hoc(({ children }) => <textarea>{children}</textarea>);

storiesOf('GitHub issues', module).add(
Copy link
Member

Choose a reason for hiding this comment

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

Naming it github issues isn't very helpful. Maybe Addon Info.HOC and withInfo('Allow Duplicate DisplayNames for HOC #1814')?

Copy link
Member Author

@Hypnosphi Hypnosphi Nov 9, 2017

Choose a reason for hiding this comment

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

I wanted to introduce a single namespace for reproductions, so that it's easy to find one

Copy link
Member

Choose a reason for hiding this comment

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

I understand, but ultimately, I still think this is a Addon Info thing. If we have 20 different Github issues and I'm making a change to Addon Info, I wouldn't think to check all the Github ones for regressions you know?

Maybe Addon Info.Github Issues and that description I had earlier?

@Hypnosphi Hypnosphi merged commit a8dc0fd into master Nov 9, 2017
@Hypnosphi Hypnosphi deleted the allow-duplicate-displaynames branch November 9, 2017 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants