Icon Component updated to v1 API (bad merge) #279
Conversation
Current coverage is 91.62%
|
|
This one I've been a little torn on. We've had a few suggestions and ideas for the API on the Icon: 1. Boolean prop <Icon square />
<Icon squareOutline />
<Icon minusSquareOutline />The camel cased versions would have to be converted to words. This means we'd need to have a map of all icon names to word forms. It also could get messy imo with regular user props in the mix. 2. Name prop <Icon name='square' />
<Icon name='square outline' />
<Icon name='minus square outline' />This is definitely cleaner to convert than boolean classes because there is no transform required. We simply include 3. Boolean prop words <Icon square />
<Icon square outline />
<Icon minus square outline />I think I like this one the least for at least two reasons. One, it increases the chance that some icon class is going to clash with some HTML attr or React prop and cause an unmaintainable conflict. 2) It also requires a lot of looping over props and for each prop looping over a list of acceptable values in order to separate icon name props from other user props. I believe I lean toward the second option, a
Thoughts? |
|
I think considering the in-roads we've been making in constraining enumerable props to defined options, as per |
- missing props added - missing examples added - Icon.Group implemented - tests updated to reflect
Sounds good to me, let's do it. By the way, one of the motivations for putting enumerables into the _meta (aside from automated tests) is a robust component explorer. I've been toying with a "playground" branch that dynamically detects component props and builds a form for trying the component out, including enum options. It is also very easy to make "presets" of the props. So, you can easily show different combinations of props. I think something like this will make it's way into the docs. Maybe version 2 or something. |
|
It's becoming apparent to me that the _meta.props should/could perhaps, become the one source of truth for components. If we were to declare all prop types in |
| const rest = getUnhandledProps(IconGroup, props) | ||
|
|
||
| return ( | ||
| <_Icon {...rest} _sdClass='sd-icon-group' _iconClass='icons'> |
There was a problem hiding this comment.
I don't think we should reuse the _Icon here. I tried all the available icon classes on the icons group and only the size classes work.
In order to preserve the propTypes validation, I think we should have a separate IconGroup component that only implements the size prop.
I think I am all for this, it has been on my mind as well. I think we can have better docs, tests, and validation with our own props definition. We could also completely do away with react-docgen that point. Because this is a large change and will not make too much end user impact, I say we wait until we finish removing jQuery and the making v1 API updates. Then, we consider how we might update the _meta to be most optimal. Let me know if you disagree or have other ideas here. |
| } | ||
|
|
||
| Icon.defaultProps = { | ||
| _sdClass: 'sd-icon', |
There was a problem hiding this comment.
Heads up, I've finished removing all sd-* classes, see #301. Rebase to master to get the update. The test for the sd-* class has been inverted to fail if any sd- class exists.
Let's remove all sd-* classes from all PRs. 👍
# Conflicts: # build/webpack.config.js
- left alligned removed as is default - Icon category included in source class name - First import on first line - Icon docs data removed from semanticUtils
- fix(ComponentExample): only intended props passed to child component - fix(IconSetExamples): irrelevant message removed - fix(semanticUtils): removed obsolete eslint-disable max-len - fix(commonTests): removed obsolete icon className assertion - fix(commonTests): removed obsolete icon className assertion
|
Looks like the |
|
There are also some strange diffs that are already in master. I know github is having some issues ATM, https://status.github.com. Though, is it possible this PR was rebased from an outdated local master? These commits are questionable:
Might need to fetch the latest master and rebase. Squashing may help as well. |
|
Had rebased from latest. Will try and clean up |
|
Can we get another rebase on this @jamiehill? |
|
Hmm, not sure how that would have happened. Always rebase after updating master. Looking into ... |
|
No worries. If you're pressed for time, you can always add me as a collaborator on your fork and I can rebase stuff as well. What ever is most helpful. I'm super appreciative of all the work and comments you've put in. |
|
There have been a number of merged PRs since this one was last updated. After rebasing to the latest master, another search for |
|
It looks like there are still some commits that shouldn't be here. I'll give this branch a rebase from my end and see if I can get it to clear up. If so, I may merge that from the CLI and manually close this PR. LMK of there is anything else you would like to do on this PR. |
|
Not sure how they've got there! If easier to add you as a collab say the word. Clearly need your help clearing this one up! |
|
Cool, let's do it. I'll save a copy of the branch just in case, then see if I can get it cleaned up. |
|
Added |
levithomason
changed the title
|
I was not able to force push the rework to your fork for some reason. I've rebased this work and reassigned the commits to you as the author in #343. I'll close this PR and we can review/merge that one instead. |
No description provided.