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

Switch bounding box is not centered #11558

Closed
chrisglein opened this issue May 4, 2023 · 12 comments
Closed

Switch bounding box is not centered #11558

chrisglein opened this issue May 4, 2023 · 12 comments
Assignees
Milestone

Comments

@chrisglein
Copy link
Member

Screenshot taken from React Native Gallery and using Accessibility Insights:
image

Contrast with this screenshot from WinUI 2 Gallery:
image

I'm guessing this is related from using that switch but not in the usual Windows style of including a label?

Originally reported here:
react-native switch paddingLeft not working cursor

@chrisglein , You can see the difference between Switch placeholders in the above screen.

Originally posted by @bahri-dev in #11554 (comment)

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label May 4, 2023
@chrisglein chrisglein added this to the Backlog milestone May 4, 2023
@acoates-ms
Copy link
Contributor

I'm guessing this is related from using that switch but not in the usual Windows style of including a label?

I think the answer is worse than that.

https://github.com/facebook/react-native/blob/80ecbdf2b28f3ee6c80a183a09c32482caa36134/packages/react-native/Libraries/Components/Switch/Switch.js#L226

Switch has a hard coded size in core/iOS, and we are not using anything different for windows.

@bahri-dev
Copy link

I think the answer is worse than that.

Switch has a hard coded size in core/iOS, and we are not using anything different for windows.

I tried to change width, height, paddingLeft in the above file. width and height work fine, but paddingLeft doesn't work.
That's why I'm asking how I can fix the placeholder issue of Switch component.

@chrisglein
Copy link
Member Author

chrisglein commented May 8, 2023

I'm a bit worried about changing the alignment for Switch on Paper and breaking an unknown number of RNW apps' look and feel in a subtle way. We're working on moving to Fabric and it has its own Switch control that at least now shares no lineage with the XAML ToggleSwitch this time around.

Question is, what size should it be? That probably depends on what the Fluent standards are?

Action here: double check that we have good sizing/centering in Fabric. Let's get some screenshots in this issue?
For Paper, I'm proposing we won't fix this.
Also should check in what (if any) design tokens or styles that FURN has for Switch.

@chrisglein chrisglein removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label May 8, 2023
@bahri-dev
Copy link

@chrisglein , the size of Slider can be changed by using width, height styles. That's why it's okay. I wish to add paddingLeft too. Then, we can change the position as similar as size. Then it's okay even though it's not center align.

@marlenecota
Copy link
Contributor

I can confirm that Switch is centered on Fabric:
image

@bahri-dev
Copy link

@marlenecota , how can I check out the above screen? Could you share the git repo?

@marlenecota
Copy link
Contributor

@bahri-dev it's the playground app that is part of this repo. Be sure to use the composition (fabric) solution.

@bahri-dev
Copy link

@marlenecota , I tried to build playground app in Win 11, but it's failed.
react-native-windows playground build error
Could you let me know how I can fix it?

@bahri-dev
Copy link

@marlenecota , I tried to use [email protected], but it's still same issue.
I compared the following files between [email protected] & playground app
node_modules\react-native-windows\Libraries\Components\Switch\Switch.js,
node_modules\react-native-windows\Libraries\Components\Switch\SwitchNativeComponent.js,
node_modules\react-native-windows\Microsoft.ReactNative\Views\SwitchViewManager.h,
node_modules\react-native-windows\Microsoft.ReactNative\Views\SwitchViewManager.cpp.
But there is no differences between them.
That's why I can't understand why [email protected] Switch doesn't work as expectedly.

@marlenecota
Copy link
Contributor

@marlenecota , I tried to use [email protected], but it's still same issue. I compared the following files between [email protected] & playground app node_modules\react-native-windows\Libraries\Components\Switch\Switch.js, node_modules\react-native-windows\Libraries\Components\Switch\SwitchNativeComponent.js, node_modules\react-native-windows\Microsoft.ReactNative\Views\SwitchViewManager.h, node_modules\react-native-windows\Microsoft.ReactNative\Views\SwitchViewManager.cpp. But there is no differences between them. That's why I can't understand why [email protected] Switch doesn't work as expectedly.

Those files are for the Paper implementation which we're not changing per Chris' comment.

The Fabric code (which has centering fixed and will eventually be the default implementation) can be found here.

@marlenecota marlenecota closed this as not planned Won't fix, can't repro, duplicate, stale Aug 8, 2023
@bahri-dev
Copy link

@marlenecota , My UWP project is based on [email protected]. How can I use the above Fabric code for Switch component?

@chrisglein chrisglein moved this from Done to Cut in RNW New Architecture Aug 22, 2023
@chrisglein
Copy link
Member Author

@marlenecota , My UWP project is based on [email protected]. How can I use the above Fabric code for Switch component?

Fabric support for RNW is still in active development. To try it out now you need to be on canary releases to get the latest and greatest, but at a minimum on version 0.72. That said, we're not recommending anyone make the migration quite yet. There's still a bunch of work to do, including getting automation around default app templates and all the support you'd need to get started.

As per my comment above, we are not going to change the placement of the existing Switch control on the mainline (Paper) versions of RNW. You'll have to work around the alignment within your app.

What we have done is confirm that this is fixed in the new version of Switch. However that will be only for Fabric, which is in an opt-in experimental state, and that'll be the case until likely 0.74+.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Cut
Development

No branches or pull requests

5 participants