-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
chore: Attachment use compose() #12674
Conversation
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 0b9847bc9bfdc1c4a0cb35ce0cfcfdc32880500b (build) |
860840a
to
a01ad8c
Compare
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Potential regressions comparing to master
Perf comparison
Perf tests with no regressions
|
@@ -34,8 +34,9 @@ export const getTelemetry = (displayName: string, telemetry: Telemetry | undefin | |||
|
|||
const useTelemetry = (displayName: string, telemetry: Telemetry | undefined): UseTelemetryResult => { | |||
const composeOptions = useComposeOptions(); | |||
const telemetryName = composeOptions?.displayNames[composeOptions?.displayNames.length - 1] || displayName; |
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.
Oops, the actual name is the last one
{Text.create(description, { | ||
defaultProps: () => ({ styles: resolvedStyles.description }), | ||
})} | ||
<div className="ui-attachment__content"> |
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.
Just curious, why we don't have AttachmentBody
component that will have the header and description props and we would create those inside that component?
packages/fluentui/react-northstar/src/components/Attachment/Attachment.tsx
Outdated
Show resolved
Hide resolved
{!_.isNil(progress) && <div className={classes.progress} style={{ width: `${progress}%` }} />} | ||
|
||
{AttachmentAction.create(action)} | ||
{!_.isNil(progress) && <div className="ui-attachment__progress" style={{ width: `${progress}%` }} />} |
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.
Why we don't have AttachmentProgress
component? I understand that you are just mapping shorthands to components, so I'm just interested whether there is a solid reason for it not being one
})} | ||
{!_.isNil(progress) && <div className={classes.progress} style={{ width: `${progress}%` }} />} | ||
|
||
{AttachmentAction.create(action)} |
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.
It's interesting that none of the sub-components need any of the parent's props :)
packages/fluentui/react-northstar/test/specs/commonTests/isConformant.tsx
Outdated
Show resolved
Hide resolved
flex: 1, | ||
}, | ||
|
||
'& .ui-attachment__progress': { |
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.
It's not obvious why this is implemented with selector. I would vote for a separate component, to keep it consistent with other slots..
…/github.com/microsoft/fluentui into chore/attachment-use-compose � Conflicts: � packages/fluentui/react-northstar/src/themes/teams-dark/componentVariables.ts
…/github.com/microsoft/fluentui into chore/attachment-use-compose � Conflicts: � packages/fluentui/CHANGELOG.md
* chore: Attachment use compose() * fix duplicate import * fix more regressions * fix UT * fix import paths * fix UTs * fix styles * fix regressions * fix acc * fix more issues * uses button variables instead * revert a change * address review comments * fix UTs * introduce body slot * update changelog * fix keys * add UT for body, add export and static field * add missing proptype * fix changelog
Pull request checklist
$ yarn change
BREAKING CHANGES
slotClassNames
removalslotClassNames
were removed fromAttachment
, please use classnames on matching components instead.styles
were moved to components from slotsaction
,description
,icon
&header
slots were moved to separate componentscontent
slots were moved to separateAttachmentBody
progress
slot are not separated anymore and should be used onroot
inattachmentStyles
Before
After
Description of changes
This PR updates
Attachment
to use insteadBox
es composed components.Adds
body
slot toAttachment
.Performance
Due enabled caching performance for rerenders was improved improved almost twice 🚀
Focus areas to test
(optional)
Microsoft Reviewers: Open in CodeFlow