Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

react-image: new shadow prop #19858

Merged
merged 8 commits into from
Sep 23, 2021

Conversation

tringakrasniqi
Copy link
Contributor

@tringakrasniqi tringakrasniqi commented Sep 20, 2021

Pull request checklist

Description of changes

Added shadow as a new prop for Image to show/remove the shadow on images.

@tringakrasniqi tringakrasniqi self-assigned this Sep 20, 2021
@tringakrasniqi tringakrasniqi added this to In progress in CXE Prague - @microsoft/cxe-prg via automation Sep 20, 2021
@tringakrasniqi tringakrasniqi requested review from a team September 20, 2021 10:53
@tringakrasniqi tringakrasniqi marked this pull request as ready for review September 20, 2021 10:54
@tringakrasniqi tringakrasniqi mentioned this pull request Sep 20, 2021
32 tasks
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 20, 2021

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
164.754 kB
46.973 kB
164.702 kB
46.954 kB
52 B
19 B
react-image
Image
9.908 kB
3.986 kB
9.856 kB
3.964 kB
52 B
22 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-components
react-components: FluentProvider & webLightTheme
35.754 kB
11.393 kB
🤖 This report was generated against 9eaa5a108ce72f6350b572ca71e496d573cd400c

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 924a287:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@size-auditor
Copy link

size-auditor bot commented Sep 20, 2021

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 9eaa5a108ce72f6350b572ca71e496d573cd400c (build)

@fabricteam
Copy link
Collaborator

fabricteam commented Sep 20, 2021

Perf Analysis (@fluentui/react)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1020 1040 5000
BaseButton mount 1057 1053 5000
Breadcrumb mount 2836 2808 1000
ButtonNext mount 517 518 5000
Checkbox mount 1817 1765 5000
CheckboxBase mount 1485 1532 5000
ChoiceGroup mount 5411 5404 5000
ComboBox mount 1085 1173 1000
CommandBar mount 11127 11036 1000
ContextualMenu mount 7193 7336 1000
DefaultButton mount 1300 1321 5000
DetailsRow mount 4242 4196 5000
DetailsRowFast mount 4266 4201 5000
DetailsRowNoStyles mount 4046 4026 5000
Dialog mount 2668 2658 1000
DocumentCardTitle mount 163 179 1000
Dropdown mount 3661 3662 5000
FluentProviderNext mount 7536 7454 5000
FluentProviderWithTheme mount 353 379 10
FluentProviderWithTheme virtual-rerender 106 112 10
FluentProviderWithTheme virtual-rerender-with-unmount 507 489 10
FocusTrapZone mount 1993 1997 5000
FocusZone mount 1984 1952 5000
IconButton mount 2034 2060 5000
Label mount 375 377 5000
Layer mount 3333 3329 5000
Link mount 526 535 5000
MakeStyles mount 1951 1934 50000
MenuButton mount 1740 1747 5000
MessageBar mount 2208 2247 5000
Nav mount 3723 3754 1000
OverflowSet mount 1245 1230 5000
Panel mount 2557 2546 1000
Persona mount 937 928 1000
Pivot mount 1581 1608 1000
PrimaryButton mount 1466 1480 5000
Rating mount 9041 9033 5000
SearchBox mount 1567 1581 5000
Shimmer mount 2960 2966 5000
Slider mount 2228 2206 5000
SpinButton mount 5599 5599 5000
Spinner mount 461 437 5000
SplitButton mount 3634 3577 5000
Stack mount 564 559 5000
StackWithIntrinsicChildren mount 2041 2028 5000
StackWithTextChildren mount 5476 5467 5000
SwatchColorPicker mount 11800 11845 5000
Tabs mount 1586 1610 1000
TagPicker mount 3085 3056 5000
TeachingBubble mount 14332 14467 5000
Text mount 472 490 5000
TextField mount 1606 1604 5000
ThemeProvider mount 1306 1290 5000
ThemeProvider virtual-rerender 638 643 5000
ThemeProvider virtual-rerender-with-unmount 2118 2088 5000
Toggle mount 951 925 5000
buttonNative mount 128 126 5000

Perf Analysis (@fluentui/react-northstar)

Perf tests with no regressions
Scenario Current PR Ticks Baseline Ticks Ratio
PortalMinimalPerf.default 215 193 1.11:1
AttachmentMinimalPerf.default 201 183 1.1:1
SkeletonMinimalPerf.default 425 391 1.09:1
ReactionMinimalPerf.default 463 430 1.08:1
BoxMinimalPerf.default 426 397 1.07:1
ButtonMinimalPerf.default 219 204 1.07:1
FlexMinimalPerf.default 344 321 1.07:1
TextMinimalPerf.default 416 388 1.07:1
CardMinimalPerf.default 693 654 1.06:1
DividerMinimalPerf.default 438 413 1.06:1
LabelMinimalPerf.default 466 439 1.06:1
CarouselMinimalPerf.default 564 537 1.05:1
ChatDuplicateMessagesPerf.default 355 338 1.05:1
HeaderSlotsPerf.default 910 865 1.05:1
AttachmentSlotsPerf.default 1249 1203 1.04:1
SegmentMinimalPerf.default 413 398 1.04:1
StatusMinimalPerf.default 808 774 1.04:1
TableMinimalPerf.default 492 471 1.04:1
TooltipMinimalPerf.default 1219 1168 1.04:1
VideoMinimalPerf.default 762 730 1.04:1
AvatarMinimalPerf.default 241 233 1.03:1
ButtonOverridesMissPerf.default 2042 1989 1.03:1
ButtonSlotsPerf.default 664 642 1.03:1
FormMinimalPerf.default 514 501 1.03:1
GridMinimalPerf.default 397 385 1.03:1
ItemLayoutMinimalPerf.default 1434 1399 1.03:1
ListCommonPerf.default 784 760 1.03:1
IconMinimalPerf.default 750 725 1.03:1
TreeMinimalPerf.default 927 903 1.03:1
TreeWith60ListItems.default 220 213 1.03:1
DialogMinimalPerf.default 853 834 1.02:1
ImageMinimalPerf.default 448 440 1.02:1
InputMinimalPerf.default 1473 1450 1.02:1
ListMinimalPerf.default 594 584 1.02:1
MenuMinimalPerf.default 974 959 1.02:1
PopupMinimalPerf.default 664 652 1.02:1
RadioGroupMinimalPerf.default 532 521 1.02:1
RefMinimalPerf.default 264 260 1.02:1
SliderMinimalPerf.default 1894 1855 1.02:1
TableManyItemsPerf.default 2237 2184 1.02:1
CustomToolbarPrototype.default 4590 4516 1.02:1
AccordionMinimalPerf.default 186 185 1.01:1
AnimationMinimalPerf.default 474 467 1.01:1
ChatMinimalPerf.default 772 766 1.01:1
CheckboxMinimalPerf.default 3049 3017 1.01:1
DatepickerMinimalPerf.default 6235 6182 1.01:1
ListWith60ListItems.default 750 743 1.01:1
MenuButtonMinimalPerf.default 1888 1861 1.01:1
ChatWithPopoverPerf.default 461 461 1:1
DropdownMinimalPerf.default 3457 3454 1:1
EmbedMinimalPerf.default 4797 4807 1:1
LayoutMinimalPerf.default 421 422 1:1
ListNestedPerf.default 641 639 1:1
LoaderMinimalPerf.default 791 789 1:1
ProviderMergeThemesPerf.default 1854 1857 1:1
ProviderMinimalPerf.default 1282 1281 1:1
ToolbarMinimalPerf.default 1097 1098 1:1
DropdownManyItemsPerf.default 796 801 0.99:1
HeaderMinimalPerf.default 426 430 0.99:1
SplitButtonMinimalPerf.default 4831 4862 0.99:1
TextAreaMinimalPerf.default 627 631 0.99:1
AlertMinimalPerf.default 314 320 0.98:1
RosterPerf.default 1364 1394 0.98:1

Copy link
Contributor

@andrefcdias andrefcdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let me know if the Screener changes are as intended and I can approve that for you too

CXE Prague - @microsoft/cxe-prg automation moved this from In progress to Reviewer approved Sep 22, 2021
@tringakrasniqi
Copy link
Contributor Author

LGTM, let me know if the Screener changes are as intended and I can approve that for you too

Thank you. Yeah they're as intended, whenever you have time please approve them.

@@ -20,6 +20,7 @@ export type ImageCommons = {
fluid?: boolean;
circular?: boolean;
rounded?: boolean;
shadow?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we have a vr-test for this prop ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point, yes it should and I'll add it. Thank you

Copy link
Member

@ling1726 ling1726 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved screener diffs, I think that removing the default box shadow behind a prop changed them all

tringakrasniqi and others added 4 commits September 23, 2021 11:10
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
@tringakrasniqi tringakrasniqi merged commit 6a0ae50 into microsoft:master Sep 23, 2021
CXE Prague - @microsoft/cxe-prg automation moved this from Reviewer approved to Done Sep 23, 2021
@tringakrasniqi tringakrasniqi deleted the image-shadow-prop branch September 23, 2021 10:44
@Hotell Hotell moved this from Done to Archive in CXE Prague - @microsoft/cxe-prg Oct 21, 2021
mlp73 pushed a commit to mlp73/fluentui that referenced this pull request Jan 17, 2022
* Added shadow prop

* Added change files

* Added sb story and vr-test

* Removing unnecessary fragment

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* Removing unnecessary fragment

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>

* Formatting fix

Co-authored-by: Oleksandr Fediashov <alexander.mcgarret@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Image by default should be shadow off (converged)
5 participants