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

Adding SimpleButton + demo scene #635

Merged
merged 27 commits into from
Feb 14, 2024

Conversation

ms-RistoRK
Copy link
Contributor

The SimpleButton structure is inspired in Mesh's MasterButtonRoot which is a simplified version of MRTK3's Action Button. Note: Materials (including shaders) are those existing in MRTK3 or its associated packages.

@ms-RistoRK ms-RistoRK marked this pull request as ready for review February 8, 2024 00:39
Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

Can you please share some screen shots of the new scenes?

@AMollis AMollis requested a review from a team February 8, 2024 01:01
@ms-RistoRK
Copy link
Contributor Author

Can you please share some screen shots of the new scenes?

Sure! Here is one taken from CanvasExampleSimpleButton scene in Editor:
image

Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

Since you added a new feature please make sure the minor versions of 'uxcomponents' and 'standardassets' have been updated since the last release (https://github.com/orgs/MixedRealityToolkit/discussions/640).

It looks like 'standardassets' version needs to be updated from 3.0.2 to 3.1.0; edit 'standassets' package.json and CHANGELOG.md.

image

It looks like `uxcomponents' version is good (min version already update), but you need to update the dependency version in 'uxcompoents' to the latest 'standardassets' version:

image

Also the fonts in the screenshot looks off. Can you make sure it's using the same font and size as the "Action Button"

@AMollis AMollis added Type: Feature Request A request for a new feature that can be included with the next minor version release. Package: Standard Assets The Project standard assets package is impacted by this issue. labels Feb 9, 2024
@AMollis AMollis added the Package: UX Components The Project ux components package is impacted by this issue. label Feb 9, 2024
@ms-RistoRK
Copy link
Contributor Author

Also the fonts in the screenshot looks off. Can you make sure it's using the same font and size as the "Action Button"

Re: Fonts size, actually, that is a feature I was trying to add. The feature is that the Font Size would auto-adjust based on the containers height and always be 25% of the container height so that the initial label ("Simple\nButton") would always have 25% of the container's height as padding when a SimpleButton is created via Editor floating menu, as in:

        [MenuItem("GameObject/UI/MRTK/Simple Button (Experimental)")]
        private static void CreateSimpleButton(MenuCommand menuCommand)
        {
            // ...
            textMeshProUGUI.fontSize = simpleButtonContentGameObject.GetComponent<RectTransform>().rect.height / 4;
            // ...
        }

... although I notice no other UIElement does font size auto-adjustment so I removed it in the last commit and made all SimpleButtons' Font size the same as ActionButtons'.
The scene now looks like this:
image
I'll work in updating the package references next.

@ms-RistoRK
Copy link
Contributor Author

It looks like 'standardassets' version needs to be updated from 3.0.2 to 3.1.0; edit 'standassets' package.json and CHANGELOG.md.

Done in latest commit.

@AMollis
Copy link
Contributor

AMollis commented Feb 10, 2024

Also the fonts in the screenshot looks off. Can you make sure it's using the same font and size as the "Action Button"

Re: Fonts size, actually, that is a feature I was trying to add. The feature is that the Font Size would auto-adjust based on the containers height and always be 25% of the container height so that the initial label ("Simple\nButton") would always have 25% of the container's height as padding when a SimpleButton is created via Editor floating menu, as in:

        [MenuItem("GameObject/UI/MRTK/Simple Button (Experimental)")]
        private static void CreateSimpleButton(MenuCommand menuCommand)
        {
            // ...
            textMeshProUGUI.fontSize = simpleButtonContentGameObject.GetComponent<RectTransform>().rect.height / 4;
            // ...
        }

... although I notice no other UIElement does font size auto-adjustment so I removed it in the last commit and made all SimpleButtons' Font size the same as ActionButtons'. The scene now looks like this: image I'll work in updating the package references next.

Yikes that font size does not look great. I suggest we change the default label to "Label", and use the same font settings has the Action Button...does this help?

image

@ms-RistoRK ms-RistoRK requested a review from AMollis February 13, 2024 23:46
@ms-RistoRK
Copy link
Contributor Author

The updated demo scene (CanvasExampleSimpleActionButton) now looks like this:
image

Copy link
Contributor

@AMollis AMollis left a comment

Choose a reason for hiding this comment

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

Please add an "MRTK Issue" to add "StateVisualizer" to the new "Simplified Button"

@ms-RistoRK ms-RistoRK merged commit 60a315d into MixedRealityToolkit:main Feb 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Package: Standard Assets The Project standard assets package is impacted by this issue. Package: UX Components The Project ux components package is impacted by this issue. Type: Feature Request A request for a new feature that can be included with the next minor version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants