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

feat(Avatar): add props interface #129

Merged
merged 2 commits into from
Aug 24, 2018
Merged

Conversation

mnajdova
Copy link
Contributor

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

-added export from the lib index.ts for the NotStrictProps type
@codecov
Copy link

codecov bot commented Aug 24, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   88.37%   88.38%   +0.01%     
==========================================
  Files          47       47              
  Lines         774      775       +1     
  Branches      110      110              
==========================================
+ Hits          684      685       +1     
  Misses         87       87              
  Partials        3        3
Impacted Files Coverage Δ
src/components/Avatar/Avatar.tsx 91.17% <100%> (ø) ⬆️
src/components/Accordion/Accordion.tsx 66% <100%> (ø) ⬆️
src/index.ts 100% <0%> (ø) ⬆️

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 4ebb956...1879004. Read the comment docs.

className?: string

/** The name used for displaying the initials of the avatar if the image is not provided. */
name?: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we consider to extract these props into IBaseProps:

/** An element type to render as (string or function). */
  as?: any
   /** Additional classes. */
  className?: string
   /** The name used for displaying the initials of the avatar if the image is not provided. */
  name?: string

so, it looks like:

interface IBaseProps {
/** An element type to render as (string or function). */
  as?: any
   /** Additional classes. */
  className?: string
   /** The name used for displaying the initials of the avatar if the image is not provided. */
  name?: string
}

export interface IAvatarPropsStrict extends IBaseProps {
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The as, className, variables, styles and accessibility are common across most of the components, but not all of them are applicable in every component. So before we define these multiple interfaces and start implementing those, we decided for now just to create the interfaces for the props and then when we will have a clear picture for all props, after merging the null checks PR and having more strict typings, we will introduce these basic interfaces and reuse them in the component's props interface. It would be easier then, because we will have all properties required and would just replace those with the appropriate interfaces.

import { ComponentVariablesInput, IComponentPartStylesInput } from '../../../types/theme'

export interface IAvatarPropsStrict {
/** The alternative text for the image used in the Avatar. */
Copy link
Contributor

Choose a reason for hiding this comment

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

a bit concerned whether we should duplicate this descriptions here now. The problem is that it will bring significant maintainability burden, as with each change to API we will need to ensure the following things:

  • examples are consistent
  • descriptions in prop types are valid
    - description of prop types correspond to these ones
  • changes in default props are properly reflected
  • handled props are properly provided

Although there is quite a lot of burden already, won't like to make the situation even worse - as it seems that there is no huge benefit in these descriptions at this stage (while I do understand that IDE suggestions will be helpful, at this point we are rather interested in ensuring type correctness).

At the same time, I am pretty sure that we will be able to address this problem (of missing comments for interface properties) later, so that there will be always single source of truth for this information.

Please, let me know about your thoughts on that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with the points above, will remove the comments on the interface as we already have the same in the prop types.

Copy link
Contributor

Choose a reason for hiding this comment

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

great - lets return to this question later and figure out generation mechanism for them :)

import { customPropTypes, UIComponent, NotStrictProps } from '../../lib'
import { ComponentVariablesInput, IComponentPartStylesInput } from '../../../types/theme'

export interface IAvatarPropsStrict {
Copy link
Contributor

Choose a reason for hiding this comment

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

just as a matter of thought - it seems that we would be able to better reflect semantics behind these prop interfaces if we would name them like:

interface IAvatarProps = {    // initially was 'IAvatarPropsStrict'
   alt?: string,
   as?: string,
   ...
}
...
// and later
class Avatar extends UIComponent<Extendable<IAvatarProps>, any> {

This approach seems to be something that introduces simplier names, better expressiveness (in terms of the goals that we would like to achieve), as well as provides generic Extendable name to generic

type Extendable<T> = T & { [key: string]: any }

rather than using specific NonStrictProps for this, in fact, generic concept.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on renaming the strict props to IAvatarProps and not having the current IAvatarProps. For the Extendable interface, I was looking for a name for the generic more describing about what is it in the context, but you are right that this might be use in other cases and the name would be inappropriate. So, in conclusion I will rename the NonStrictProps to Extendable and will get rid of having two interfaces for the props in the components. Will address in this PR the changes for the Accordion component as well.

…faces and replacing NotStrictProps with Extendable
@mnajdova mnajdova merged commit 46cd27f into master Aug 24, 2018
@levithomason levithomason deleted the feat/avatar-prop-interface branch October 29, 2019 22:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants