Skip to content

feat(react-persona): Adding initial implementation#24749

Merged
sopranopillow merged 18 commits intomicrosoft:masterfrom
sopranopillow:persona-prototype
Sep 28, 2022
Merged

feat(react-persona): Adding initial implementation#24749
sopranopillow merged 18 commits intomicrosoft:masterfrom
sopranopillow:persona-prototype

Conversation

@sopranopillow
Copy link
Contributor

@sopranopillow sopranopillow commented Sep 10, 2022 •
edited
Loading

This PR adds the initial implementation of Persona.

Note: Styles are still a WIP, this PR is mainly to get the logic down.

Related Issue(s)

Fixes #24640

@sopranopillow sopranopillow self-assigned this Sep 10, 2022
@fabricteam
Copy link
Collaborator

fabricteam commented Sep 10, 2022 •
edited
Loading

📊 Bundle size report

🤖 This report was generated against 615a471e1cab0e63c8d586a49031601f8749c7d6

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 10, 2022 •
edited
Loading

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 19eb6ba:

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

@size-auditor
Copy link

size-auditor bot commented Sep 10, 2022 •
edited
Loading

Asset size changes

⚠️ Insufficient baseline data to detect size changes

Unable to find bundle size details for Baseline commit: 0f74ad4

Possible causes

  • The baseline build 0f74ad4 is broken
  • The Size Auditor run for the baseline build 0f74ad4 was not triggered

Recommendations

  • Please merge your branch for this Pull request with the latest master build and commit your changes once again

@sopranopillow sopranopillow marked this pull request as ready for review September 12, 2022 23:28
@sopranopillow sopranopillow requested a review from a team as a code owner September 12, 2022 23:28
@sopranopillow sopranopillow mentioned this pull request Sep 15, 2022
Merged
behowell
behowell reviewed Sep 16, 2022
View reviewed changes
@sopranopillow sopranopillow mentioned this pull request Sep 27, 2022
Closed
22 tasks
behowell
behowell approved these changes Sep 27, 2022
View reviewed changes
Copy link
Contributor

@behowell behowell left a comment

Choose a reason for hiding this comment

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

Added some comments, but I'm going to approve it so you can merge after addressing them. Thanks!

packages/react-components/react-persona/src/components/Persona/usePersonaStyles.ts
let primaryTextSize;
let optionalTextSize;

if (fixed) {
Copy link
Contributor

@behowell behowell Sep 27, 2022

Choose a reason for hiding this comment

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

The behavior of the fixed/scaling sizing is still a bit more complicated than I would expect. I don't think it should hold up this PR, but we might want to circle back with design to see if we can simplify the behavior there.

My main concern is that there's "magic" behavior around the scaling of the text size when you set the Avatar's size; and that you can't access that same text sizing behavior when you're using auto-sized Avatar.

It seems like instead of tying the Avatar's size to the text size, that Persona itself should have a size prop like small/medium/large, which would be be used to control the text size. Then, the default size of the Avatar would be based on the combination of Persona's size and the number of lines of text (instead of just the number of lines of text like it is today). IMO that would make the API simpler (less magic), and more powerful (more direct control over the text size).

Copy link
Contributor

@behowell behowell Sep 27, 2022

Choose a reason for hiding this comment

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

Another reason I prefer the Persona size prop is that it keeps the "data flow" in one direction: from Persona to Avatar (by having it control the default size). As it is now, the styling of Persona depends on a prop on the avatar slot, which is potentially confusing that the prop on one slot affects other slots.

Copy link
Contributor

@behowell behowell Sep 27, 2022

Choose a reason for hiding this comment

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

Followed up offline; will talk to design and address in a future PR.

packages/react-components/react-persona/src/components/Persona/usePersonaStyles.ts
Comment on lines +22 to +23
gridAutoColumns: 'max-content',
gridAutoRows: 'max-content',
Copy link
Contributor

@behowell behowell Sep 27, 2022

Choose a reason for hiding this comment

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

Out of curiosity, why is max-content needed instead of the default? Do you need it for both columns and rows?

Copy link
Contributor Author

@sopranopillow sopranopillow Sep 28, 2022

Choose a reason for hiding this comment

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

gridAutoRows: max-content is needed because when you have a larger height than the actual needed, it will stretch and mess up the spacing:
image

for the columns it's the same issue, but it's worse since the coin will stretch and the text will be off:
image

@sopranopillow sopranopillow merged commit a93e52d into microsoft:master Sep 28, 2022
@sopranopillow sopranopillow deleted the persona-prototype branch September 28, 2022 20:52
NotWoods pushed a commit to NotWoods/fluentui that referenced this pull request Nov 18, 2022
@sopranopillow @NotWoods
feat(react-persona): Adding initial implementation (microsoft#24749)
4eed673 
* feat: Adding initial implementation.

* updating stories

* updating position prop

* making Avatar the primary slot

* updating tests

* adding requested changes

* adding final changes

* removing grid areas

* updating styles

* min sync

* updating dependencies

* updating tests

* restoring readme

* adding a better description to slots

* adding requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@behowell behowell behowell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@sopranopillow sopranopillow

Labels

Component: Persona Fluent UI react-components (v9)

Projects

None yet

Milestone

July Project Cycle Q3 2022

Development

Successfully merging this pull request may close these issues.

Persona: Add initial implementation

3 participants

@sopranopillow @fabricteam @behowell