feat(Loader): add support for SVG animations, update in Teams theme#1097
feat(Loader): add support for SVG animations, update in Teams theme#1097layershifter merged 15 commits intomasterfrom
Conversation
layershifter
requested review from
dvdzkwsk,
kuzhelov,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1097 +/- ##
=========================================
- Coverage 82.11% 82.02% -0.1%
=========================================
Files 717 720 +3
Lines 8584 8605 +21
Branches 1169 1169
=========================================
+ Hits 7049 7058 +9
- Misses 1518 1530 +12
Partials 17 17
Continue to review full report at Codecov.
|
| medium: pxToRem(57.6), | ||
| large: pxToRem(115.2), | ||
| larger: pxToRem(115.2), | ||
| largest: pxToRem(115.2), |
There was a problem hiding this comment.
@codepretty can you please provide correct sizes for these elements to make them less magic?
layershifter
force-pushed
the
feat/loader-svg
branch
from
a725cc8 to
e4a1e35
Compare
| const { indicator, label, svg } = this.props | ||
| const { visible } = this.state | ||
|
|
||
| const svgElement = Box.create(svg, { |
There was a problem hiding this comment.
I am wondering whether the better shorthand for the svg would be the Icon component.. Can you elaborate why we are using the Box here?
There was a problem hiding this comment.
Can you please clarify why it can be Icon? For example, this loader is an extra big SVG:
And actually, I don't see any way how spinners below can be used as icons:
There was a problem hiding this comment.
Make sense, I mention icon only because it is an svg
| variables: v, | ||
| }: ComponentStyleFunctionParam<LoaderProps, LoaderVariables>): ICSSInJSStyle => ({ | ||
| // Reset existing styles from base theme | ||
| animationName: 'none', |
There was a problem hiding this comment.
I wish we didn't have to reset here all existing styles from the base theme, but I don't have any other proposal. The styles for the base theme seems reasonable for it's usage
There was a problem hiding this comment.
Yep, it's absolutely not funny, but I don't have a better idea now...
| return { | ||
| ...outerAnimation, | ||
|
|
||
| ':before': { |
There was a problem hiding this comment.
How does this works? why we have animations for the svg and the :before? Can we somehow move this to the indicator and the svg accordingly?
There was a problem hiding this comment.
Is this because of the svg that is used?
There was a problem hiding this comment.
Idea with svg slot works fine, but Teams loader requires specific markup, i.e need one more element. Here is match:
I'm opened for better idea...
…tardust-ui/react into feat/loader-svg # Conflicts: # CHANGELOG.md
2 participants
Fixes #1038.