Button component added #13

igorls · 2017-06-27T19:52:08Z

New class Material.Button according to the guidelines, adjustments are still needed. Current options are Raised/Flat, Enabled/Disabled, Background and Ink (ripple) colors.

Demo added to the MainView.ux

Also adding Roboto default font:
Default Material typeface should be Roboto per the guidelines, changed Material.Text class to comply with https://material.io/guidelines/style/typography.html

eksperts · 2017-06-27T20:27:53Z

Components/Button.ux

@@ -0,0 +1,71 @@
+<Panel ux:Class="Material.Button" Alignment="Center" Enabled="true" Raised="true" Height="48" MinWidth="88" Padding="8,0" HitTestMode="LocalBoundsAndChildren" BackgroundColor="White" InkColor="#ccc">


I would use HitTestMode="LocalBounds" instead, that seems sufficient

eksperts · 2017-06-27T20:29:29Z

Components/Button.ux

+	<float4 ux:Property="BackgroundColor"/>
+	<float4 ux:Property="InkColor"/>
+
+	<Rectangle ux:Name="RaisedButton_body" ZOffset="1" Height="36" Padding="8,0" CornerRadius="2">


When assigning a ux:Name to a node, the value should start with a lowercase letter and be in camelCase. This here, for example, should be ux:Name="raisedButtonBody".

And more of a question rather than a request for changes - is the ZOffset necessary? Can't you instead reorder the nodes so that the ones on top would be on top of the others in UX file?

I reordered, but the ZOffset is necessary actually

Why is it necessary? I don't see any triggers involved in manipulating them, so I don't immediately see the reason.

@igorls I tested and the ZOffsets do not seem to be necessary at all. You only need to put the nodes in the right order, just like layers in photoshop - the ones on top in UX are rendered on top of others.

eksperts · 2017-06-27T20:33:27Z

Components/Button.ux

+	<float4 ux:Property="InkColor"/>
+
+	<Rectangle ux:Name="RaisedButton_body" ZOffset="1" Height="36" Padding="8,0" CornerRadius="2">
+		<SolidColor Color="{ReadProperty this.BackgroundColor}" />


You don't need to prefix these properties with an explicit this.*, since they are referring to the whole ux:Class we are currently in and this.* is implied.

eksperts · 2017-06-27T20:33:53Z

Components/Button.ux

+
+	<Rectangle ux:Name="RaisedButton_body" ZOffset="1" Height="36" Padding="8,0" CornerRadius="2">
+		<SolidColor Color="{ReadProperty this.BackgroundColor}" />
+		<WhileTrue Value="{ReadProperty this.Raised}">


You don't need to prefix these properties with an explicit this.*, since they are referring to the whole ux:Class we are currently in and this.* is implied.

eksperts · 2017-06-27T20:34:32Z

Components/Button.ux

+	</Rectangle>
+
+	<Rectangle ux:Name="innerPanel" ZOffset="3" Height="{ReadProperty RaisedButton_body.Height}" Padding="{ReadProperty RaisedButton_body.Padding}" CornerRadius="{ReadProperty RaisedButton_body.CornerRadius}" ClipToBounds="true">
+		<Text Value="{ReadProperty this.Text}" Font="RobotoMedium" ux:Name="text" Alignment="Center" FontSize="14" />


You don't need to prefix these properties with an explicit this.*, since they are referring to the whole ux:Class we are currently in and this.* is implied.

eksperts · 2017-06-27T20:34:40Z

Components/Button.ux

+		<Text Value="{ReadProperty this.Text}" Font="RobotoMedium" ux:Name="text" Alignment="Center" FontSize="14" />
+	</Rectangle>
+
+	<WhileTrue Value="{ReadProperty this.Enabled}" Invert="true">


You don't need to prefix these properties with an explicit this.*, since they are referring to the whole ux:Class we are currently in and this.* is implied.

Instead of using <WhileTrue Invert="true"> can't you use a plain <WhileFalse>?

eksperts · 2017-06-27T20:35:41Z

Components/Button.ux

+	</Rectangle>
+
+	<Rectangle ux:Name="innerPanel" ZOffset="3" Height="{ReadProperty RaisedButton_body.Height}" Padding="{ReadProperty RaisedButton_body.Padding}" CornerRadius="{ReadProperty RaisedButton_body.CornerRadius}" ClipToBounds="true">
+		<Text Value="{ReadProperty this.Text}" Font="RobotoMedium" ux:Name="text" Alignment="Center" FontSize="14" />


ux:Name should always be the first property on a node, followed by Value in case of <Text />.

eksperts · 2017-06-27T20:36:51Z

Components/Button.ux

+
+	</WhileTrue>
+
+	<WhileTrue Value="{ReadProperty this.Enabled}">


You don't need to prefix these properties with an explicit this.*, since they are referring to the whole ux:Class we are currently in and this.* is implied.

eksperts · 2017-06-27T20:39:28Z

Components/Button.ux

+				<Set scaleAnimator.Duration="0.3" AtProgress="0" />
+				<Set showInk.Value="true" AtProgress="0" />
+				<Set showInk.Value="false" AtProgress="1" />
+				<Set scaleAnimator.Duration="0.6" AtProgress="1" />


It would be nice if these triggers were grouped together by the AtProgress value (0/1), otherwise it's hard to read what happens when.

eksperts · 2017-06-27T20:40:28Z

MainView.ux

 			</Material.AppBar>
+
+			<Rectangle Dock="Top" Margin="5">
+				<StackLayout/>


Why use a Rectangle with a StackLayout when you can just have a StackPanel?

I just wanted to use a stroke to separate the button example area from other upcoming examples

That still should be done differently though. Go with a StackPanel and we'll figure it out from there.

igorls · 2017-06-27T21:40:51Z

all requested changes were fixed, exception to the ZOffsets that were actually needed.

Default Material typeface should be Roboto per the guidelines, changed Material.Text class to comply (https://material.io/guidelines/style/typography.html)

New class Material.Button according to the guidelines, adjustments are still needed. Current options are Raised/Flat Enabled/Disabled, as long as Background and Ink (ripple) colors. Demo added to the MainView.ux

eksperts · 2017-06-28T12:09:56Z

MainView.ux

+				</StackPanel>
+				<StackPanel Orientation="Horizontal">
+					<Material.Button Text="Flat Button - Enabled" Raised="false" Enabled="true" BackgroundColor="#FFFFFF00" InkColor="Material.Blue300"/>
+					<Material.Button Text="Flat Button - Disabled" Raised="false" Enabled="false" BackgroundColor="#FFFFFF00"/>


Is the background on these fully transparent on purpose? Wouldn't you want to reuse the same color as on the raised buttons, Material.Blue200?

I chose to put no background on those to show that the ink effect applies to the area in case of a flat button, what do you think?

Duckers · 2017-06-28T09:28:50Z

Style/Fonts.ux

@@ -0,0 +1,5 @@
+<Panel ux:Class="Material.Fonts">


Please don't add these fonts, it severely bloats the app.

This theme will use the platform's default font, which is Roboto on Android and San Francisco on iOS.

Bundling full Roboto will add 10MB to the app.

Ok, no problem, I also had a doubt about this

igorls · 2017-06-29T15:40:28Z

I think it's better for us to hold that PR until I replace my own implementation of the ink effect with the same one @Duckers has used in the other components, main reason being the glitch with the CornerRadius non-zero. Also to focus all effects on the same uno class, avoiding problems in the future

eksperts requested changes Jun 27, 2017

View reviewed changes

igorls added 4 commits June 27, 2017 19:44

Adding Roboto default font

a73dddf

Default Material typeface should be Roboto per the guidelines, changed Material.Text class to comply (https://material.io/guidelines/style/typography.html)

Added Material.Button (work in progress)

58ae445

New class Material.Button according to the guidelines, adjustments are still needed. Current options are Raised/Flat Enabled/Disabled, as long as Background and Ink (ripple) colors. Demo added to the MainView.ux

fix

b51569c

small adjustments according to requested changes

92dd446

eksperts reviewed Jun 28, 2017

View reviewed changes

Duckers requested changes Jun 28, 2017

View reviewed changes

Reverting to default fonts as requested

926f564

		@@ -0,0 +1,71 @@
		<Panel ux:Class="Material.Button" Alignment="Center" Enabled="true" Raised="true" Height="48" MinWidth="88" Padding="8,0" HitTestMode="LocalBoundsAndChildren" BackgroundColor="White" InkColor="#ccc">

Button component added #13

Are you sure you want to change the base?

Button component added #13

Uh oh!

Conversation

igorls commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eksperts Jun 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorls commented Jun 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorls commented Jun 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eksperts Jun 27, 2017 •

edited

Loading