Skip to content
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

[RFC] Support prop aliases extensions #126

Closed
4 tasks
kettanaito opened this issue Mar 7, 2019 · 4 comments · May be fixed by #171
Closed
4 tasks

[RFC] Support prop aliases extensions #126

kettanaito opened this issue Mar 7, 2019 · 4 comments · May be fixed by #171
Labels
enhancement New feature or request help wanted Extra attention is needed needs:discussion Further information is requested
Milestone

Comments

@kettanaito
Copy link
Owner

kettanaito commented Mar 7, 2019

What:

I propose to consider support of custom prop aliases (or aliases extensions).

Why:

  • To suit different projects' needs
  • To have more power with responsive props, while not violating the core principle of Atomic layout (if shipped by default)

How:

  • Developers can create a custom prop by using the exposed API on the Layout level:
import Layout from 'atomic-layout'

Layout.configure({
  propAliases: {
    textAlign: {
      output: ['text-align'],
      transformValue: optionalTransformer,
    }
  }
})
  • All custom prop aliases are responsive by default
  • Custom prop alias supports value transformer to be able to transform the supplied value

  • Put down an agreed API
  • Extend the Layout class
  • Expose default utils (i.e. transformNumeric) for developers to reuse the library's logic when writing custom aliases
  • Provide tests
@kettanaito kettanaito added enhancement New feature or request needs:discussion Further information is requested labels Mar 7, 2019
@kettanaito kettanaito added this to the 1.0 milestone Mar 7, 2019
@kettanaito
Copy link
Owner Author

kettanaito commented Apr 22, 2019

Implementing a PoC for this, and stumbling upon a circular dependency issue between Layout and its import of defaultOptions, and propAliases and its import of transformNumeric.

Underlying cause is that transformNumeric imports Layout for default unit check. That creates the following import order:

Layout -> defaultOptions -> propAliases -> transformNumeric -> Layout (circle)

I would love to make transformNumeric pure, accepting default unit via arguments. This would resolve the circular dependency, and make this util more reliable.


This may burst a more broad discussion on how to effectively distribute global layout settings throughout the library's internals.

@Hermanya
Copy link

I don't know any library that exposes a way to change its api. How is this documentable/ typable?

I think it's more common to have a separate npm package with its own api and documentation. Like bootstrap-atomic-layout, which api would be more inspired by the bootstrap utils api.

@kettanaito
Copy link
Owner Author

It's not a change, it's extension of the API with the options you need. API, in this case Responsive props API, behaves as per spec in documentation, but you can configure your custom aliases for any purpose. It's very similar to such API from styled-system.

@kettanaito kettanaito modified the milestones: 1.0, 0.9.0 May 11, 2019
@kettanaito kettanaito self-assigned this May 21, 2019
@kettanaito kettanaito removed their assignment Jul 8, 2019
@kettanaito kettanaito modified the milestones: 0.9.0, 1.0, 0.10.0 Jul 10, 2019
@kettanaito kettanaito added the help wanted Extra attention is needed label Aug 5, 2019
@kettanaito
Copy link
Owner Author

Once useResponsiveProps React hook has been introduced in 0.8.0, support for the custom prop aliases has been put under question.

Although custom prop aliases can be of practical use, I'm strongly convinced that it would do more harm, if introduced. Atomic layout encourages a separation of concerns by splitting the visual styling and positional styling. It felt uncomfortable for me at first too, but doing project after project following this separation I can hardly notice it now. Moreover, styling has become significantly easier.

Why custom prop aliases is not a good idea?

Because it allows (and, thus, encourages) mixing visual and positional styling. This contradicts the core message of the library, which is the separation of concerns. On practice that would make Atomic layout a formless box to stuff all your styles into. I don't find this as a good styling encouragement, or even an implementation approach.

How to substitute custom prop aliases?

Nevertheless, I can share the need to benefit from integrating certain custom styles with Atomic layout. During my work it mainly comes down to applying certain styles conditionally using Responsive props.

Using useResponsiveProps hook can simplify responsive values declaration and make it aligned with the breakpoints:

import styled from 'styled-components'
import { useResponsiveProps } from 'atomic-layout'

const StyledText = styled.p`
  font-size: ${({ size }) => size}rem;
`
const Text = (props) => {
  // This processed the incoming props and returns any responsive props.
  // So it's possible to suffix any props with a breakpoint name, thus, making
  // any prop responsive.
  const { size } = useResponsiveProps(props)

  return <StyledText size={size} />
}

// Usage example
<Text size={1} sizeMd={2} sizeLg={3} />

With the introduction of #194 the declaration may become less verbose. The only difference with this way of applying responsive values is that the mapping between a prop name and what it actually does is up to the end consumer (which is rather a good thing).

Summary

That being said, I don't think that custom prop aliases is a good idea. Atomic layout should support a sufficient amount of prop aliases (CSS properties) for effective layout positioning. If any props are missing, please file an issue to add them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed needs:discussion Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants