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

feat(config)!: use object parameter instead of array #10

Merged
merged 43 commits into from
Dec 27, 2020

Conversation

joanrm20
Copy link
Contributor

Hey @rbardini,

So I started to work on #8. Let me know if this is in the right path of what you were aiming to solve. :)

What I did:

  1. In order to be able to keep array and object config I'm normalizing just the entries/items so the rest of the internal logic will work in the same way.
  2. I'm proposing the keyword sizes as part of the paddings global object, just so it looks a bit more readable from the config outside. Please let me know what do you think on this one? :)
{
  paddings: {
    sizes: {
      small: { name: 'Small', value: '16px' },
      medium: { name: 'Medium', value: '32px' },
      largin: { name: 'Large', value: '64px' },
    },
    defaultPadding: 'medium',
  }
}

Pending:

  1. I still would like to know how you'd like to approach the config from the examples and docs point of view? Should I create a different example? Set the example to object as default or simply just update the docs with the new option?

Alternative Option:

  1. Instead of normalizing the entries we could diverge the logic and treat it internally as object or array for every condition. Although this might require some more code as it will need a validation for every condition checking whether it's the Array one or the Object and do finds or object "picks" from the provisioned configurations. I can provide code example of this one if needed. :)
  2. In the same line of thought of 1, I found a small concern on that approach. Even if we use object notation sometime to find "x" value we'll still might need to turn the config into an array to find the previously picked item.

src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
Copy link
Owner

@rbardini rbardini left a comment

Choose a reason for hiding this comment

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

Thanks, @joanrm20! I added a few comments, mostly to follow the conventions of the new Backgrounds addon API.

As for the docs, you can update all README and demo examples to use the object format. Migration tips will be added to the GitHub release once merged.

src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
@rbardini rbardini self-assigned this Jul 18, 2020
@rbardini rbardini added the enhancement New feature or request label Jul 18, 2020
Copy link
Owner

@rbardini rbardini left a comment

Choose a reason for hiding this comment

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

@joanrm20 I added a few more suggestions, especially regarding the API. Let me know what you think 🙂

Also, we need to update the README examples, but it's better to decide the final API first.

src/containers/PaddingSelector.tsx Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
src/containers/PaddingSelector.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
src/helpers.tsx Outdated Show resolved Hide resolved
@rbardini rbardini linked an issue Jul 18, 2020 that may be closed by this pull request
package.json Outdated Show resolved Hide resolved
@rbardini rbardini changed the title feat(config): use paddings config object instead of array feat(config)!: use paddings config object instead of array Dec 15, 2020
@rbardini
Copy link
Owner

rbardini commented Dec 20, 2020

@joanrm20 I just merged #20 to run the main workflow runs on pull requests, and #21 to highlight the selected padding option. Could you please sync your branch to integrate these changes?

@joanrm20
Copy link
Contributor Author

hey @rbardini, done. I have merged and test your latest changes. Please check.

Copy link
Owner

@rbardini rbardini left a comment

Choose a reason for hiding this comment

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

LGTM! Will merge it soon 👍

Copy link
Owner

@rbardini rbardini left a comment

Choose a reason for hiding this comment

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

Sorry, found two lines to be removed before we merge this, otherwise looks good 🙂

src/preset/defaultParameters.tsx Outdated Show resolved Hide resolved
example/stories/2-Card.stories.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@joanrm20
Copy link
Contributor Author

joanrm20 commented Dec 25, 2020

hey @rbardini, I've updated it. Please take a look.

package.json Outdated Show resolved Hide resolved
@rbardini rbardini changed the title feat(config)!: use paddings config object instead of array feat(config)!: use object parameter instead of array Dec 27, 2020
@rbardini rbardini merged commit e5a8405 into rbardini:master Dec 27, 2020
@github-actions
Copy link

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use paddings config object instead of array
2 participants