fix: Refactored List component so type and display options are handled separately (#512) #513
Conversation
…er props handle display (#512)
Co-authored-by: Francis Fernandez <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #513 +/- ##
=======================================
Coverage 99.20% 99.20%
=======================================
Files 110 110
Lines 1256 1261 +5
Branches 172 172
=======================================
+ Hits 1246 1251 +5
Misses 5 5
Partials 5 5 ☔ View full report in Codecov by Sentry. |
| variant = "unordered", | ||
| ...attrs | ||
| }) => { | ||
| const classes = classNames( | ||
| inline ? `${classPrefix}-inline` : classPrefix, | ||
| variant === "plain" && `${classPrefix}-plain`, | ||
| className | ||
| plain ? `${classPrefix}-plain` : "", |
There was a problem hiding this comment.
| plain ? `${classPrefix}-plain` : "", | |
| (plain || variant === "plain") ? `${classPrefix}-plain` : "", |
Would help preserve backwards compatibility if the variant plain is deprecated and not actually removed.
There was a problem hiding this comment.
We shouldn't have a "plain" prop, because then it could lead to some weird situations:
<List plain variant="description">There should just be the "variant" prop.
There was a problem hiding this comment.
Having plain as a variant is what caused the original issue (#512). While having it as a separate prop may cause strange combinations they don't directly conflict outside and it would be on the users if they want the combination and if so how to handle any strangeness.
There was a problem hiding this comment.
FYI: I added a comment to #512 that I'm hoping may inform what happens in this PR.
Or maybe it just opens up a rabbit hole. Feel free to just shut me up if you want.
| ); | ||
| const ListTag = variant === "ordered" ? "ol" : "ul"; | ||
| const ListTag = getElement(variant); |
There was a problem hiding this comment.
Rather than a function, you could do this:
// At the top of the module:
const variantTagMap = {
description: 'dl',
ordered: 'ol',
plain: 'ul',
unordered: 'ul'
};
// Here:
const ListTag = variantTagMap[variant];Up to you if you want to change it. But I feel like these mapping objects tend to be cleaner than functions and switch statements.
There was a problem hiding this comment.
Also, I wonder if we need to be a little more internally consistent on something. Here, we use both "tag" and "element" interchangeably. Then PR #511 uses the "component" prop. Maybe that's something we clean up at some point.
There was a problem hiding this comment.
Likewise, HTML conventions tends to use a "type" attribute similarly to how we're using the "variant" prop here. So again, I don't know if this is something to discuss.
There was a problem hiding this comment.
I agree with standardizing on component (I've seen that on other projects). HTML using type as an attribute throws a wrench in our library components.l I think it was alerts we had a problem with in the alpha and switched away from type.
| variant = "unordered", | ||
| ...attrs | ||
| }) => { | ||
| const classes = classNames( | ||
| inline ? `${classPrefix}-inline` : classPrefix, | ||
| variant === "plain" && `${classPrefix}-plain`, | ||
| className | ||
| plain ? `${classPrefix}-plain` : "", |
There was a problem hiding this comment.
We shouldn't have a "plain" prop, because then it could lead to some weird situations:
<List plain variant="description">There should just be the "variant" prop.
|
My original intent was to keep existing functionality but just in a deprecated state until a major release. However forgot I needed to keep the original line and have readded it. |
No description provided.