seas-computing / mark-one

A UI component library for building React Apps (in development)
https://seas-computing.github.io/mark-one/
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

Feature/236 eslint fixes #69

Closed jonseitz closed 4 years ago

jonseitz commented 4 years ago

Sorry this is such a huge pull request. The key things to look at would be:

The theme stuff is probably the most significant. Basically, the way we were setting up typescript for styled-components didn't match the official docs and it was creating a lot of additional work for us. So I've removed the BaseTheme type from the library altogether, then moved the type definitions into the DefaultTheme interface within the styled-components module in styled.d.ts. *The upshot of that is that we no longer need to explicitly type the theme prop, or even include it in a `Propsinterface, becausestyled-componentsis already implicitly including athemeprop of typeDefaultTheme` in every component.**

The new typescript eslint rules were also generating some errors about unsafely accessing any values in places where we were getting a value from the theme with a variable (e.g., theme.background.color[variant].medium). This seemed annoying at first, but it is actually an issue that should be addressed for those rare circumstatnce where a value for variant might be set at runtime to a value for which we don't have an object. The best workaround I could come up with was the fromTheme function I added to Theme/utils.ts, which returns a function to access values from the theme (it has to be done this way to allow for overriding the theme in other projects). The resulting syntax is a little unwieldy:

  color: ${({ variant, theme }) => (
    fromTheme('color', 'background', variant, 'medium')({ theme })
  )};

But because the function returned by fromTheme will only return a string or null, we can safely access dynamic values. That function-returning-a-function pattern also lets us streamline our regular theme accessors since we no longer need to define a function that destructures theme from props. Example:

font-weight: ${fromTheme('font', 'body', 'weight')};
natalynyu commented 4 years ago

I also found this comment from 'goatslacker,' a collaborator, saying if you need to break early that a loop is ok - https://github.com/airbnb/javascript/issues/417

image

jonseitz commented 4 years ago

OK, these latest changes should fix all the type checking issues and we've removed the loop, so I think all the points have been addressed.