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

Simplify Responsive prop #279

Closed
couds opened this issue Aug 20, 2020 · 3 comments
Closed

Simplify Responsive prop #279

couds opened this issue Aug 20, 2020 · 3 comments

Comments

@couds
Copy link
Owner

couds commented Aug 20, 2020

Currently the responsive prop is way to complex, quite nested objects, I'm thinking on flat the object and separate it into several different props. what do you guys think?

This will be a breaking change so if possible I will try to push it to V4.

What I have in mind is something like this:

Create one prop for each breakpoints mobile, tablet, touch, desktop, widescreen, fullhd with this shape.

PropTypes.shape({
    display: PropTypes.oneOf(displays),
    hide: PropTypes.oneOf([true, false, 'only']),
    textSize: PropTypes.oneOf([1, 2, 3, 4, 5, 6]),
    textAlignment: PropTypes.oneOf([
      'centered',
      'justified',
      'left',
      'right',
      'centered-only',
      'justified-only',
      'left-only',
      'right-only',
    ]),
  }),
@kennethnym kennethnym mentioned this issue Aug 21, 2020
37 tasks
@kennethnym
Copy link
Collaborator

@couds will the syntax be something like this?

<Button
  mobile={{
    hide: true,
    display: 'flex',
    textSize: 6,
  }}
/>

if so, then i prefer something like onMobile, or onDesktop. Also, I think we should have separate props for viewport-specific helpers, like onDesktopOnly and onMobileOnly.

<Button
  onMobile={{
    hide: true,
    display: 'flex',
  }}
  onDesktopOnly={{
    textSize: 6,
  }}
/>

@couds
Copy link
Owner Author

couds commented Oct 11, 2020

Hi @MrCreeper1008 I will think about the dmobile/deskropOnly.

What is for sure is that the prop name will not start with on because normally that is for event listener and could be cconfusing

@kennethnym
Copy link
Collaborator

Fixed in commit 1c5ce9a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants