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

Refactor ui #30

Merged
merged 6 commits into from Jun 17, 2014

Conversation

Projects
None yet
2 participants
@GaryDeng
Copy link
Contributor

GaryDeng commented Jun 9, 2014

No description provided.

GaryDeng added some commits Jun 9, 2014

Merge branch 'master' of https://github.com/bigbluebutton/bbb-air-client
 into modify-launcher-icons

Conflicts:
	src/Main.mxml
	src/org/bigbluebutton/view/navigation/pages/chat/ChatViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/chatrooms/ChatRoomsViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/common/MenuButtons.mxml
	src/org/bigbluebutton/view/navigation/pages/participants/ParticipantsViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/presentation/PresentationViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/profile/ProfileViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/selectparticipant/SelectParticipantViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/userdetails/UserDetaisViewBase.mxml
	src/org/bigbluebutton/view/navigation/pages/videochat/VideoChatViewBase.mxml
}

s|Button.ProfileSettingsButtonStyle

This comment has been minimized.

@capilkey

capilkey Jun 12, 2014

Member

should be a lowercase p

ui|NavigationButton.deskshareBtnStyle
{
height:44;

This comment has been minimized.

@capilkey

capilkey Jun 12, 2014

Member

All of the NavigationButton classes have the same height value. I think it would be better if you move the height out to a higher level.

width: 44;
height: 44;
backgroundColor: #FFFFFF;

This comment has been minimized.

@capilkey

capilkey Jun 12, 2014

Member

I think this selected/unselected pair can be improved upon. The only difference is the backgroundColor

s|Label#userName
{
fontFamily: Georgia;

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

Do we want a different font for just the name?

<s:Button id="newMessages" buttonMode="false" horizontalCenter="0"
styleName="newMessagesButtonStyle" verticalCenter="0"
visible.ButtonState="{_buttonStateVisible}"
visible.NormalState="{_normalStateVisible}"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

Are the two states actually need for the item renderer? It seems like you could just set the visibility directly in the data setter.

</s:Group>

<chatrooms:ChatRoomsList id="chatroomslist" width="100%" height="100%" />

<common:MenuButtonsView width="100%" height="42"/>
<common:MenuButtonsView />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

On some of the main ViewBase classes the MenuButtonsView instance has width="100%" and some don't. I think they all should have the width removed because the default is 100%.

<ui:NavigationButton id="presentationBtn0" width="100%" navigateTo="{[PagesENUM.PRESENTATION]}" styleName="presentationBtnStyle" height="{getStyle('height')}"/>
<ui:NavigationButton id="videochatBtn0" navigateTo="{[PagesENUM.VIDEO_CHAT]}" width="100%" styleName="videochatBtnStyle" height="{getStyle('height')}"/>
<ui:NavigationButton id="chatBtn0" navigateTo="{[PagesENUM.CHATROOMS,PagesENUM.CHAT,PagesENUM.SELECT_PARTICIPANT]}" width="100%" styleName="chatBtnStyle" height="{getStyle('height')}"/>
<ui:NavigationButton id="participantsBtn0" navigateTo="{[PagesENUM.PARTICIPANTS,PagesENUM.USER_DETAIS]}" width="100%" styleName="participantsBtnStyle" height="{getStyle('height')}"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think setting the heights in mxml directly conflicts with the values given in the CSS. You should set one or the other not both.

horizontalCenter.CONNECTION_DROPPED="0" verticalCenter.CONNECTION_DROPPED="200"
horizontalCenter.USER_KICKED_OUT="0" verticalCenter.USER_KICKED_OUT="200"
horizontalCenter.USER_LOGGED_OUT="0" verticalCenter.USER_LOGGED_OUT="200"/>
<s:Button id="exitButton0" width="50%" content="Exit" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I know that it's not related to CSS, but the "Exit" string should be moved to the locales.

</s:Group>
<s:Button id="soundIcon" width="5%" styleName="soundIconStyle" />
<s:Button styleName="arrowRightStyle" width="5%" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

Couldn't all of the these Buttons be changed to Images? I think something similar to the LoginPageViewBase should be possible.

<s:Button id="cameraIcon" width="5%" styleName="cameraIconStyle" />
<s:Group left="0" width="5%">
<s:Button id="micIcon" styleName="micIconStyle" />
<s:Button id="micOffIcon" styleName="micOffIconStyle" visible="false" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

You should be able to use styles to change the icon instead of keeping two instances around and showing/hiding one of them.

<s:VerticalLayout gap="0" paddingBottom="0" paddingLeft="0" paddingRight="0" paddingTop="0" />
</s:layout>
<s:Label id="userName" width="100%" />
<s:Line width="100%" height="0">
<s:stroke>
<s:SolidColorStroke color="0x000000" weight="2"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

The weight of the stroke needs to be moved to CSS because we need thicker lines on higher dpi devices

<s:VerticalLayout gap="0" paddingBottom="0" paddingLeft="0" paddingRight="0" paddingTop="0" />
</s:layout>
<s:Button id="shareCameraBtn0" width="100%" alpha="1" buttonMode="true"
useHandCursor="true" styleName="ProfileSettingsButtonStyle" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

When the object is a Button you don't need to set buttonMode=true, useHandCursor=true, and alpha=1 because all of those are the default values. The same applies to the other buttons on this page.

</s:layout>
<s:Button id="shareCameraBtn0" width="100%" alpha="1" buttonMode="true"
useHandCursor="true" styleName="ProfileSettingsButtonStyle" />
<s:Line height="0" width="100%" >
<s:stroke>
<s:SolidColorStroke color="0x000000" weight="2"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

This stroke weight needs to be moved to CSS

view.micOnOffText.text = ResourceManager.getInstance().getString('resources', userMe.voiceJoined? 'profile.settings.mic.on':'profile.settings.mic.off');
view.raiseHandText.text = ResourceManager.getInstance().getString('resources', userMe.raiseHand ?'profile.settings.handLower' : 'profile.settings.handRaise');
view.shareMicButton.label = ResourceManager.getInstance().getString('resources', userMe.voiceJoined? 'profile.settings.mic.on':'profile.settings.mic.off');
view.raiseHandButton.label = ResourceManager.getInstance().getString('resources', userMe.raiseHand ?'profile.settings.handLower' : 'profile.settings.handRaise');

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think the setting of the above two labels can be moved to the ViewBase instead because it is always set to those values

paddingLeft="0"
paddingRight="0"
paddingTop="0"
paddingTop="20"

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

Having values for padding in the files will lead to differences at different DPIs

verticalCenter="0"/>
<s:Label id="statusText" width="187" horizontalCenter="43" text="Moderator"
textAlign="left" verticalCenter="0"/>
<userdetails:HRule width="90%"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

There's an HRule class defined in the userdetails package? Isn't HRule a Flex class?

<s:Button id="cameraIcon" styleName="cameraIconStyle" height="100%" />
<s:Group height="100%">
<s:Button id="micIcon" styleName="micIconStyle" height="100%" left="0"/>
<s:Button id="micOffIcon" styleName="micOffIconStyle" visible="false" height="100%" left="0"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think you can reduce these to one instance by taking advantage of CSS

<s:Button id="micIcon" styleName="micIconStyle" height="100%" left="0"/>
<s:Button id="micOffIcon" styleName="micOffIconStyle" visible="false" height="100%" left="0"/>
</s:Group>
<s:Button id="soundIcon" styleName="soundIconStyle" height="100%" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think all of these Buttons that are actually just informational images can be changed to Images in the same way as the participants item renderer

paddingLeft="0"
paddingRight="0"
paddingTop="10"
paddingTop="30"

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

Having a value of 30 here will lead to differences at the various DPI possibilities

</s:Group>
<ui:SwapCameraButton id="swapCameraBtn" width="30" height="30" />
<ui:MicButton id="microphoneBtn" width="30" height="30"/>
<ui:ProfileButton id="profileBtn" navigateTo="{[PagesENUM.PROFILE]}"/>

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I don't think the navigateTo needs to be set here

<s:Group id="videoGroup0" width="100%" height="100%">
<s:Label id="noVideoMessage0" width="90%" height="100"
fontSize="20" horizontalCenter="0"
<s:Label id="noVideoMessage0" width="90%" height="100%"

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think there's a style defined for the no video message, but it's not actually used on the page

</s:states>
<s:Rect width="{hostComponent.getStyle('width')}" height="{hostComponent.getStyle('height')}">
<s:stroke>
<s:SolidColorStroke color="0x000000" weight="1" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think the stroke weight should be moved to CSS


<s:Rect width="100%" height="{hostComponent.getStyle('height')}">
<s:stroke>
<s:SolidColorStroke color="0x000000" weight="1" />

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

The weight should be moved to CSS

@playerversion AIR 1.5
@productversion Flex 4
-->
<s:SparkSkin xmlns:fx="http://ns.adobe.com/mxml/2009" xmlns:s="library://ns.adobe.com/flex/spark"

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

It looks like you copied the RadioButtonSkin from the SDK source. Did you make any changes to the source?

this.addEventListener(MouseEvent.CLICK, onClick);
var selected:State = new State({name : "selected"});
var unselected:State = new State({name : "unselected"});
selected.overrides = [new SetStyle(this,"backgroundColor", this.getStyle('selectedBackgroundColor') )];

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

When the NavigationButton state is changed from selected to unselected the object's background colour is still grey. I think you need to create an override for the unselected state as well.

@@ -1,10 +1,11 @@
<?xml version="1.0" encoding="utf-8"?>
<s:Group xmlns:fx="http://ns.adobe.com/mxml/2009" height="30" width="30"
xmlns:s="library://ns.adobe.com/flex/spark" >
<s:Group xmlns:fx="http://ns.adobe.com/mxml/2009"

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think the SwapCameraBase can be changed to inherit from the Button class directly instead of being a wrapper for an actual Button object

}

s|RadioButton.radioButtonStyle

This comment has been minimized.

@capilkey

capilkey Jun 13, 2014

Member

I think you should remove the style selector from this so that it affects all of the RadioButton objects in the application to keep the look and feel the same

@GaryDeng GaryDeng closed this Jun 16, 2014

@GaryDeng GaryDeng deleted the SenecaCDOT-BigBlueButton:refactor-ui branch Jun 16, 2014

@capilkey capilkey restored the SenecaCDOT-BigBlueButton:refactor-ui branch Jun 16, 2014

@GaryDeng GaryDeng reopened this Jun 16, 2014

capilkey added a commit that referenced this pull request Jun 17, 2014

@capilkey capilkey merged commit 51ab0ff into bigbluebutton:master Jun 17, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.