This repository has been archived by the owner on Mar 4, 2020. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 53
feat(Accordion): add props interface #33
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
6028777
fix: Component interfaces extend React attributes
bmdalex 7813cac
-fixing types
1616e3c
-fixed images in the avatar
b2b0f87
-added variables prop to the Label component
799677e
-added props interface to the Label component
4800164
-fixes
0fc3605
-reverted changes in the avatar examples
3615ba7
Merge branch 'master' into fix/component-interfaces
9a39575
-fixed variables type to work with object too
c2e377f
-changed the PropTypes in the Label component
c7979b6
Merge branch 'master' into fix/component-interfaces
da7abe4
-added props interface for the accordion component
21f79e4
-extracting NotStringProps generic type
bc5fdd3
-more specific function definition for the Accordion onTitleClick pro…
a517d51
-updated types for the variables and styles
276a8c6
-removing children from prop interface
aa26eb0
Merge branch 'master' into fix/component-interfaces
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
export type NotStrictProps<T> = T & { | ||
[key: string]: any | ||
} |
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.
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.
accessibility
,styles
,variables
,className
are common for all components, shouldn't we extract them to separate interface. If so, then to how many?Does the same apply to
children
?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 was thinking about the same thing. For the children, they should be provided even from React itself so I am not sure if we should specify them here at all. Not sure if the same applies for the className as well. For the rest, I would propose having the styles and variables in one interface, because for all themed components both would apply, and having the accessibility in other interface, because I am not sure if it is applicable to all components. We should think about the 'as' property as well. Having this said, I am kind a worry that we would end up with lot's of interfaces, which can become confusing, but at the same time I agree that it could be good for refactoring, if we need to add some common property or rename existing common property in the future for some reason. Any thoughts on this? @kuzhelov @levithomason
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.
agree with the points mentioned above. However, my vote would rather be to do it after strict rtpes will be introduced for all components, so that at this point we will have clear picture around what are the general parts and what are the exceptions/edge cases.
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.
Got it! Will wait then for final approve for this PR in order for it to be merged.