Button Refactors Final Final #16772
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,39 +8,35 @@ | |
| :bottom 0}) | ||
|
|
||
| (defn icon-style | ||
|
There was a problem hiding this comment. icon background color is not actually a thing afaik so I removed 👍 |
||
| [{:keys [icon-container-size icon-container-rounded? | ||
| icon-size margin-left margin-right]}] | ||
| (cond-> (merge {:margin-left margin-left | ||
| :margin-right margin-right | ||
| :align-items :center | ||
| :justify-content :center}) | ||
| icon-container-size | ||
| (assoc :width icon-container-size :height icon-container-size) | ||
| icon-container-rounded? | ||
| (assoc :border-radius (/ (or icon-container-size icon-size) 2)))) | ||
|
|
||
| (defn icon-left-icon-style | ||
| [{:keys [override-margins size icon-container-size icon-container-rounded? | ||
| icon-size]}] | ||
| (icon-style | ||
| {:margin-left (or (get override-margins :left) | ||
| (if (= size 40) 12 8)) | ||
| :margin-right (or (get override-margins :right) 4) | ||
| :icon-container-size icon-container-size | ||
| :icon-container-rounded? icon-container-rounded? | ||
| :icon-size icon-size})) | ||
|
|
||
| (defn icon-right-icon-style | ||
| [{:keys [override-margins size icon-container-size icon-container-rounded? | ||
| icon-size]}] | ||
| (icon-style {:margin-left (or (get override-margins :left) 4) | ||
| :margin-right (or (get override-margins :right) | ||
| (if (= size 40) 12 8)) | ||
| :icon-container-size icon-container-size | ||
| :icon-container-rounded? icon-container-rounded? | ||
| :icon-size icon-size})) | ||
|
|
||
|
|
@@ -53,54 +49,67 @@ | |
| 56 12 | ||
| 40 12 | ||
| 32 10 | ||
| 24 8 | ||
| 12))}) | ||
|
|
||
| (defn style-container | ||
| [{:keys [size disabled? border-radius background-color border-color icon-only? icon-top | ||
| icon-left icon-right]}] | ||
| (merge {:height size | ||
| :align-items :center | ||
| :justify-content :center | ||
| :flex-direction (if icon-top :column :row) | ||
| :padding-horizontal (when-not (or icon-only? icon-left icon-right) | ||
| (case size | ||
| 56 (if border-color 10 11) | ||
|
There was a problem hiding this comment. fixes padding for icon above variant |
||
| 40 16 | ||
| 32 12 | ||
| 24 7 | ||
| 16)) | ||
| :padding-left (when-not (or icon-only? icon-left) | ||
| (case size | ||
| 56 nil | ||
|
There was a problem hiding this comment. A bit weird to see a condition that returns nil, but I understand you probably kept Still, it's a bit rare to see in Clojure a I know I keep repeating this, but Even if we gracefully handle the exception (which we don't now), I believe most users prefer to not see failures in apps, and are more receptive to a styling issue if it's not grotesque of course. There was a problem hiding this comment. yep, well padding-left will overwrite it otherwise - sure I can update the default case to nil 👍 but there is default values here? 🤔 There was a problem hiding this comment. Great point @J-Son89. If there's no default value, the ideal scenario to me is to log and return nil. Unmatched conditions that will look wrong shouldn't go unnoticed. Of course, one may argue that nobody will see the logs, but it surely helps devs to more proactively catch these issues instead of real users, which would be bad. Trusting that the code will pass only valid sizes now and in the future is a recipe for user bugs with This idea to return nil to avoid the exception with There was a problem hiding this comment. Oh well this is quite specific to setting up this component because here padding horizontal can clash with padding left and padding right. Ideally this code could be written to just avoid the use of padding horizontal here but that would involve me redesigning the button styling and so I don't want to invest that effort as there is a lot variations to check and I have invested a lot of time in this component already. But there is definitely a cleaner approach here |
||
| 40 16 | ||
| 32 12 | ||
| 24 8 | ||
| 16)) | ||
| :padding-right (when-not (or icon-only? icon-right) | ||
| (case size | ||
| 56 nil | ||
| 40 16 | ||
| 32 12 | ||
| 24 8 | ||
| 16)) | ||
| :padding-top (when-not (or icon-only? icon-left icon-right) | ||
| (case size | ||
| 56 0 | ||
| 40 (if border-color 8 9) | ||
|
There was a problem hiding this comment. fixes alignment for outline variant |
||
| 32 (if border-color 4 5) | ||
| 24 (if border-color 0 1) | ||
| (if border-color 8 9))) | ||
| :padding-bottom (when-not (or icon-only? icon-left icon-right) | ||
| (case size | ||
| 56 0 | ||
| 40 9 | ||
| 32 5 | ||
| 24 4 | ||
| 9)) | ||
| :overflow :hidden | ||
| :background-color background-color | ||
| :border-radius (if border-radius | ||
| border-radius | ||
| (case size | ||
| 56 12 | ||
| 40 12 | ||
| 32 10 | ||
| 24 8 | ||
| 12)) | ||
| :border-color border-color | ||
| :border-width (when border-color 1)} | ||
| (when icon-only? | ||
| {:width size}) | ||
| (when border-color | ||
| {:border-color border-color | ||
| :border-width 1}) | ||
| (when disabled? | ||
| {:opacity 0.3}))) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icon-secondary-coloris not needed as the icon is the label color for icon only variant 👍