Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

feat(Label): add props interface #148

Merged
merged 6 commits into from Aug 29, 2018
Merged

Conversation

kuzhelov
Copy link
Contributor

This PR introduces props interface for the Label component, as part of #117

types/utils.d.ts Outdated
@@ -12,3 +12,4 @@ export type Extendable<T> = T & {

export type ItemShorthand = React.ReactNode | object | (React.ReactNode | object)[]
export type ReactChildren = React.ReactNodeArray | React.ReactNode
export type SyntheticEventHandler<TProps> = (event: React.SyntheticEvent, data: TProps) => void
Copy link
Member

Choose a reason for hiding this comment

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

Propose we call this something like StardustEventHandler. The signature here is unique to Stardust and does not apply all synthetic event handlers.

Copy link
Contributor Author

@kuzhelov kuzhelov Aug 28, 2018

Choose a reason for hiding this comment

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

agree with the reasons provided, but would rather suggest to use ComponentEventHandler then. The problem I see with using StardustEventHandler is that with the approach of prefixing library-specific types with 'Stardust' we would potentially need to introduce this prefix to other types too (ItemShorthand, as an example?). To me it seems reasonable to assume that all types that are introduced as part of the library are Stardust ones, and there is no additional need to emphasize this :)

Let me, please, change it to ComponentEventHandler for now and make a promise to rename it in case it won't work well for us

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

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

One suggestion, see comment.

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

Merging #148 into master will decrease coverage by 0.02%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #148      +/-   ##
==========================================
- Coverage    69.2%   69.17%   -0.03%     
==========================================
  Files         101      101              
  Lines        1341     1340       -1     
  Branches      251      250       -1     
==========================================
- Hits          928      927       -1     
  Misses        411      411              
  Partials        2        2
Impacted Files Coverage Δ
src/components/Accordion/AccordionTitle.tsx 65% <ø> (ø) ⬆️
src/components/Accordion/Accordion.tsx 66% <ø> (ø) ⬆️
src/components/Button/Button.tsx 93.93% <ø> (ø) ⬆️
src/components/Input/Input.tsx 78.33% <ø> (ø) ⬆️
src/components/Accordion/AccordionContent.tsx 80% <ø> (ø) ⬆️
src/themes/teams/components/Label/labelStyles.ts 37.5% <0%> (ø) ⬆️
src/components/Label/Label.tsx 100% <100%> (ø) ⬆️

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 87093d9...44514f0. Read the comment docs.

@kuzhelov kuzhelov merged commit 9df6889 into master Aug 29, 2018
@kuzhelov kuzhelov deleted the feat/add-types-to-label-props branch August 29, 2018 00:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants