Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: element component + subcomponent type conventions #1273
fix: element component + subcomponent type conventions #1273
Changes from 34 commits
768d5db
1e6bd41
9a02d1a
e417b2e
d2c4080
fdd66de
6a64999
2880dfc
490d5dd
0ddd5f5
0f76ecf
948b4ce
afe876c
2d38efa
20bb8fe
c9f7464
80f00df
044bbe1
ed6afc9
201c72b
336bb9e
c39055c
12bdff7
1ca4bf1
307f689
446a9e9
fb084a5
96d896b
47623c3
b946422
aacc24c
f4c3881
5782dad
dbdf6ea
0296143
f06df5d
102c90d
4b96955
deff0e5
4107c0a
db114b2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little confused here.. "should be set to the exported component name", would that be
Element
orElementComponent
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only exported component is
Element
– so that should be thedisplayName
. Do I need to take another crack at the verbiage here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I may have taken it too literally with how it is worded? The linting work correctly so it shouldn't be an issue... I just understood it as (example):
Instead of what we are actually doing:
Since
Accordion is what is being exported. Possibly making a distinction between
element's and
styled-components` would help.. not too sure frankly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your (example) can't happen – it breaks both displayName linting and JSDoc parsing.
We actually need the "actually doing" version. And the point is to highlight the necessary
AccordionComponent.displayName = 'Accordion';
mismatch – which satisfies both linting and provides the necessary connective tissue for JSDoc parsing viagarden cmd-docgen
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When would
@ignore
usage be appropriate? I did noticeButtonGroup
sisSelected
prop as one usage withinreact-components
, would this hide functionality or is it intended for the internal API?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rare notation hides props that should only be used internally and not intended for external use. Should I state that here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would help to state that, maybe using the internal props as an example would frame its purpose better.