Skip to content

Tokenize button Android/iOS#2337

Merged
ayush547 merged 19 commits into
microsoft:mainfrom
ayush547:tokenize-android-button
Nov 28, 2022
Merged

Tokenize button Android/iOS#2337
ayush547 merged 19 commits into
microsoft:mainfrom
ayush547:tokenize-android-button

Conversation

@ayush547
Copy link
Copy Markdown
Contributor

@ayush547 ayush547 commented Nov 16, 2022

Platforms Impacted

  • iOS
  • macOS
  • win32 (Office)
  • windows
  • android

Description of changes

  • Added paddingStart and paddingEnd to LayoutTokens.

  • Completed FAB component support for text styling and appearances.

  • Expanded Button appearance options to support 'Accent' and 'Outline' as required by mobile endpoints.
    Button existed in FURN with 3 possible “appearance” values -

    Default (No appearance value passed as prop) –
    image
    Primary –
    image
    Subtle –
    image

    While the designer guidelines for Button appearance on mobile is as follows –
    Accent –
    image
    Outline –
    image
    Subtle –
    image

    Since props are shared across platform, consumers could pass appearances that are not implemented for the specific platforms, to get around this the following mapping is done based on similarity between the options -

    Accent -> Primary on All platforms
    Default (No appearance passed) -> Primary on Mobile
    Outline -> Default on Non-Mobile platforms
    (Colors in the snapshots for the buttons were updated because of default mapping to primary now)

  • Updated FAB 'icon' prop to a required prop. Jest snapshot tests were accordingly updated.

  • 'iconOnly' prop was added to all required samples to apply intended styling.

  • Added Android token values for Button and FAB.

  • Updated spec files for Button and FAB.

iOS Changes -

  • Appearance Mapping & FAB completion carries over to iOS as well
  • Tester App changes and Spec updates is left for when token value are updated for iOS

Verification

Dark Light
MicrosoftTeams-image (5) MicrosoftTeams-image (8)
MicrosoftTeams-image (6) MicrosoftTeams-image (7)

Pull request checklist

This PR has considered (when applicable):

  • Automated Tests
  • Documentation and examples
  • Keyboard Accessibility
  • Voiceover
  • Internationalization and Right-to-left Layouts

@ayush547 ayush547 requested a review from a team as a code owner November 16, 2022 09:39
@rurikoaraki
Copy link
Copy Markdown
Collaborator

This is a large PR. In the future, to make this easier to review and to minimize disruption if a change needs to be backed out, I suggest keeping your changes smaller. For example, you could split the Button and FAB changes into separate PRs, or the support for inner borders as its own PR.

Comment thread packages/components/Button/src/Button.types.ts Outdated
Comment thread apps/fluent-tester/src/TestComponents/Shadow/ShadowButtonTestSection.tsx Outdated
Comment thread packages/components/Button/src/Button.types.ts Outdated
Comment thread packages/components/Button/src/FAB/FAB.types.ts
Comment thread packages/components/Button/src/Button.styling.ts Outdated
Comment thread packages/components/Button/src/ButtonTokens.android.ts Outdated
Comment thread packages/components/Button/src/FAB/FAB.test.tsx Outdated
Comment thread packages/components/Button/src/FAB/FAB.test.tsx Outdated
Comment thread packages/components/Button/src/FAB/FAB.types.ts Outdated
@rurikoaraki
Copy link
Copy Markdown
Collaborator

rurikoaraki commented Nov 16, 2022

Curious if this change is meant to span both Android and iOS?

@lyzhan7 and @Saadnajmi FYI

@ayush547
Copy link
Copy Markdown
Contributor Author

Curious if this change is meant to span both Android and iOS?

@lyzhan7 and @Saadnajmi FYI

Button and FAB for iOS and Android have similar requirements. Have made all the changes with the intent that it carries over to iOS as well

Copy link
Copy Markdown
Contributor

@ankraj12 ankraj12 left a comment

Choose a reason for hiding this comment

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

I would suggest following Fluent android guidelines in tester for android. We can find a way to best handle what's not supported, but from what works it should be as what is recommended for android.

Comment thread apps/fluent-tester/src/TestComponents/Button/ButtonVariantTestSection.tsx Outdated
Comment thread packages/components/Button/src/__snapshots__/Button.test.tsx.snap
Comment thread packages/components/Button/src/Button.styling.ts Outdated
@Saadnajmi
Copy link
Copy Markdown
Collaborator

Curious if this change is meant to span both Android and iOS?
@lyzhan7 and @Saadnajmi FYI

Button and FAB for iOS and Android have similar requirements. Have made all the changes with the intent that it carries over to iOS as well

I asked my coworkers who work on FluentUI Apple (aka, native iOS). Indeed, we are not aware of spec differences between the iOS and Android Fluent 2 buttons, so most of this should carry over. I imagine that's not an explicit goal of this PR, but good to know!

@ayush547 ayush547 closed this Nov 22, 2022
@ayush547 ayush547 force-pushed the tokenize-android-button branch from fb154c7 to 448ebea Compare November 22, 2022 07:25
@ayush547 ayush547 reopened this Nov 22, 2022
Comment thread packages/components/Button/SPEC.md Outdated
Copy link
Copy Markdown
Contributor

@lyzhan7 lyzhan7 left a comment

Choose a reason for hiding this comment

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

Can we update the PR description "Platforms Impacted" to indicate that this affects iOS as well? And the title as well. Also looks like the PR still refers to "ascent" instead of "accent"

I can help test this on iOS and can send screenshots of what your PR looks like on iOS. Some of the changes seem android specific (i.e. a lot of new .android files) - it might be helpful to have a list of the expected changes for iOS, since it seems like iOS had a different starting point from Android

@ayush547 ayush547 changed the title Tokenize android button Tokenize button Android/iOS Nov 23, 2022
@ayush547
Copy link
Copy Markdown
Contributor Author

Can we update the PR description "Platforms Impacted" to indicate that this affects iOS as well? And the title as well. Also looks like the PR still refers to "ascent" instead of "accent"

I can help test this on iOS and can send screenshots of what your PR looks like on iOS. Some of the changes seem android specific (i.e. a lot of new .android files) - it might be helpful to have a list of the expected changes for iOS, since it seems like iOS had a different starting point from Android

Have updated the PR description.
Have not touched the tester app/specs for iOS, though all the code present in the .android files can be used directly for tester app on iOS without any changes.

@ankraj12 ankraj12 self-requested a review November 24, 2022 13:36
Copy link
Copy Markdown
Contributor

@ankraj12 ankraj12 left a comment

Choose a reason for hiding this comment

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

Feedback over mappings.

Comment thread packages/components/Button/src/Button.styling.ts Outdated
@ayush547 ayush547 requested a review from ankraj12 November 26, 2022 05:23
@ayush547 ayush547 merged commit 890ff26 into microsoft:main Nov 28, 2022
@ayush547 ayush547 deleted the tokenize-android-button branch November 28, 2022 11:31
@ayush547 ayush547 mentioned this pull request Nov 29, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants