fix(Avatar, Image): fixing fluid prop in Image and applying it in the Avatar component #77
Conversation
mnajdova
requested review from
dvdzkwsk,
kuzhelov and
levithomason
as code owners
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
==========================================
- Coverage 85.97% 85.89% -0.08%
==========================================
Files 76 76
Lines 1112 1113 +1
Branches 219 220 +1
==========================================
Hits 956 956
- Misses 149 150 +1
Partials 7 7
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #77 +/- ##
=======================================
Coverage 86.29% 86.29%
=======================================
Files 39 39
Lines 664 664
Branches 99 99
=======================================
Hits 573 573
Misses 88 88
Partials 3 3
Continue to review full report at Codecov.
|
| @@ -4,11 +4,11 @@ export default { | |||
| root: ({ props, variables }) => ({ | |||
| display: 'inline-block', | |||
| verticalAlign: 'middle', | |||
| width: variables.width || (props.fluid && '100%'), | |||
There was a problem hiding this comment.
this, actually, imposes other problem. Variables are more specific than props (in terms of the styling aspects targeting), so for the following case where conflicting things are requested
<Image fluid variables={{ width: '100px' }} />the result should be that Image will have 100px width. Essentially, the following principle should be preserved: more specific customization option should always win. With the proposed change it won't be the case anymore.
There was a problem hiding this comment.
I agree for the width, will change it, but for the avatar then we must provide something like variables.avatarSize || (props.fluid && '100%') || pxToRem(32)' and we can have that variable to be undefined by default.
There was a problem hiding this comment.
After discussing, the decision was to have the prop be the first statement in the condition, because otherwise if the theme is specifying the default width, the fluid prop will never be correctly applied.
| imageAvatar: ({ props: { size } }) => ({ | ||
| width: pxToRem(getAvatarDimension(size)), | ||
| height: pxToRem(getAvatarDimension(size)), | ||
| imageAvatar: () => ({ | ||
| verticalAlign: 'top !important', |
There was a problem hiding this comment.
😕 hmmm, let's not use important if we can avoid it. Betting this was copied cruft from SUIR? Let's try removing it please.
# Conflicts: # src/themes/teams/components/Avatar/avatarStyles.ts
|
currently variables are passed down the tree by Avatar component - we should prevent it to happen by introducing them to handledProps set. The same story applies to 'styles'.
On a long-term, we should ensure that this is not happening by means of conformance tests (something that we've already discussed) - let me, please, take care of tests for this. |
Image
Applying the fluid prop to the avatar variations for the image.
Avatar
Using the fluid prop for the image component.
TODO