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

Add Expander native component (WinUI 2.6) #756

Merged
41 commits merged into from Aug 13, 2021

Conversation

amdingler
Copy link
Contributor

@amdingler amdingler commented Jul 1, 2021

Platforms Impacted

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

Description of changes

Adding a native windows Expander control (WinUI 2.6) to FURN.

  • packages/experimental/Expander/windows: Contains native module code for the Expander control
  • packages/experimental/Expander/src: Contains code to compose the native view into a component

Notable design choices:

  • packages/experimental/Expander/windows/ReactNativeExpander/ExpanderView.idl: ExpanderView inherits from a ContentPresenter, and the Expander control is displayed from within the ContentPresenter control. This is because we are unable to inherit from the Expander control directly.

Future work:

  • Resolve layout issues between Yoga and XAML layout systems within the RNW framework. Conflicting layout systems caused the following issues and work arounds in the Expander implementation:
    • Expander requires a fixed collapsed and expanded height
    • height must be exposed as props to allow the proper collapsed/expanded height changes. If the user tries to use the height prop, it is overridden. Can potentially fix this using token states to update the Expander height.
    • Expander does not account for height changes caused by window resizing
    • User cannot interact with non-native components within the header. In this scenario, the Expander will expand instead.
  • Remove implicit ordering of Expander header and content. The user should be able to specify which child element is the header and which is the content.

Verification

Tested on a React Native for Windows v0.64.x tester app. Expander was tested for light mode, dark mode, and keyboard accessibility.

Basic Expander

Basic_Expander_Demo.mp4

Full Expander Functionality

Expander_Full_Demo.mp4

Dark Mode

Dark_Mode_Expander

Pull request checklist

This PR has considered (when applicable):

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

@amdingler amdingler marked this pull request as draft July 1, 2021 17:21
@amdingler amdingler changed the title DRAFT: Add Expander native component (WinUI 2.6) Add Expander native component (WinUI 2.6) Jul 26, 2021
@Saadnajmi
Copy link
Collaborator

Something I forgot to mention: If you could add screenshots / videos to your PR, it would be really helpful to understand how the component looks / should behave when reviewing. And is good historical data when looking through git history :D

@amdingler amdingler marked this pull request as ready for review July 27, 2021 23:47
Copy link

@rectified95 rectified95 left a comment

Choose a reason for hiding this comment

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

Great stuff! Left a few nit comments. Also, make sure you finish off ReplaceChild() and resolve the conflict in yarn.lock

if (userProps.onCollapsing !== undefined) {
userProps.onCollapsing();
}
// Need to delay the height change so that the animation runs
Copy link
Contributor

@PPatBoyd PPatBoyd Aug 6, 2021

Choose a reason for hiding this comment

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

is this a pattern rn-windows folks have been taking to generate layout animations w/o using LayoutAnimation [due to limitations e.g. its global nature] ? This is a 175ms wait on the JS thread, am I reading that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we added a wait on the JS thread to allow enough time for the native animation to complete before changing the height of the Expander.

Copy link
Member

Choose a reason for hiding this comment

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

@PPatBoyd RNW does not support RN layout animations. All animations here are XAML animations.

*/
chevronForeground?: ColorValue;
/*
* Chevron background color on pointer over
Copy link
Contributor

Choose a reason for hiding this comment

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

the chevron gets different background and foreground colors from the rest of the control's background, e.g. we can't reuse BackgroundTokens ?

the variations should be handled in styling and only need chevronBackground chevronForeground

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pulled these tokens from values that can be put in the ResourceDictionary, so they can all be different colors.

@amdingler
Copy link
Contributor Author

@Saadnajmi Just added the test files.

@Saadnajmi Saadnajmi added the AutoMerge 🔁 Automatically merge when PR requirements met label Aug 13, 2021
@ghost
Copy link

ghost commented Aug 13, 2021

Hello @Saadnajmi!

Because this pull request has the AutoMerge :repeat: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 364c4b6 into microsoft:master Aug 13, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge 🔁 Automatically merge when PR requirements met
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants