Basic project cleanup tasks #15

sthewissen · 2017-09-22T15:32:08Z

No description provided.

jamesmontemagno · 2017-09-22T16:59:25Z

I had a few comments, mostly on making sure not to break UWP/Android.

jamesmontemagno · 2017-09-22T16:53:51Z

MVP/MVP/MVP/ViewModels/MyProfileViewModel.cs

@@ -75,7 +76,7 @@ public static MyProfileViewModel Instance
        private string _strMeImage = "Me.png";
        private string _strNotificationsImage = "Notifications.png";
        private string _strSupportImage = "Support.png";
-        private string _strSettingsImage = "Settings.png";
+        private string _strSettingsImage = "icon_settings.png";


We need to be careful here. I don't like how these are all setup and we should standardize on a name for all icons.

I'm not even sure why these are being bound. Looks like its done to have the same image be culture specific, but there are better ways to do that I'd say (e.g. a ValueConverter). For cleanliness sake I wouldn't want this kind of stuff in the business part (where it is now) since it's a UI thing.

I added a new document, we should to through ALL of the images, rename them and then do a OnPlatform for UWP for the Assets folder where they actually belong.

I agree, they should NOT be in there at all (the viewmodels that is). It doesn't feel right and isn't necessary.

jamesmontemagno · 2017-09-22T16:54:29Z

MVP/MVPUI/App.xaml.cs

@@ -19,6 +19,8 @@ public partial class App : Application

        #region Public Fields

+        public static App Instance;


App already has an instance to itself with Application.Current

Updated code using Application.Current instead and removed Instance.

jamesmontemagno · 2017-09-22T16:55:04Z

MVP/MVPUI/LiveIdLogOn.xaml

-<?xml version="1.0" encoding="utf-8" ?>
-<ContentPage xmlns="http://xamarin.com/schemas/2014/forms"
+<?xml version="1.0" encoding="utf-8" ?>
+<local:ModalPage Title="Sign in" xmlns="http://xamarin.com/schemas/2014/forms"


Why do we have a modal page? I am going to wrap all modal pages in a navigation page with a close button on the top right.

This was done for iOS because it behaves badly when modal pages and status/navigation bar colors are concerned. What would happen is when you tapped the Sign in button (white status bar) the modal page shows (black status bar) but going back the statusbar won't turn white again. This is fixed through a custom renderer based on ModalPage. If there's a way to check if ContentPage is currently shown modal I'd love to hear it.

So this wont be a problem if we wrap the page in a NavigationPage! I added this to the guidelines. This will ensure that it is correct.

Shouldn't "Close"/"Cancel" go in the top left for iOS modals? My understanding is top right is for things like "Save"/"Done" or to take some type of action.

Most likely should. Will change it ASAP.

Yeah, so I think we should not add a custom renderer for that as it isn't really needed and will cause some weird stuff on other platforms.

On the bottom of a page can be Save button and then on the top right will be Close/Cancel always. This just simplifies it across all platforms

- Renamed existing images - Trimmed away a lot of images - Moved stuff into asset catalogs

jamesmontemagno · 2017-09-22T18:37:03Z

So @sthewissen, we will also need to update UWP and Android icons names to match so they run correctly to. Additionally since all UWP images should be in the "Assets" folder we should update all of them to use the OnPlatform so they display correct.

Are you up to do that in this PR?

sthewissen · 2017-09-22T19:13:49Z

Sure, can do that. I still have a whole lot of images in the iOS side of things that I think aren't being used. Let me get rid of those first so I have some sort of baseline to tweak the other ones to :)

Also this kind of stuff: https://github.com/Microsoft/mvp/blob/master/MVP/MVP/MVP/Helpers/CommonConstants.cs#L91

I can leave it in there for now, might take some effort to get rid of it all. Especially since I have no way of testing all the functionality since there's no "dummy" MVP in there.

jamesmontemagno · 2017-09-22T19:24:07Z

I think the contribution type icons that are hard coded should be in a converter.

I appreciate it and we should remove any images that aren't needed from others as it is confusing.

sthewissen · 2017-09-22T21:47:19Z

Moved the type icon to a converter, but it's still working on some string matching magic which I don't consider ideal. For now it's moved as is.
Cleaned up all unused images for all platforms (tested on Droid and iOS, WinPhone verification needed).
Removed the activities part on the profile page and moved it to contributions tab

jamesmontemagno · 2017-09-23T05:56:08Z

I should be able to test out Windows 10 on Sunday.

These can simply be put in the XAML.

jamesmontemagno · 2017-09-26T19:19:11Z

Was able to pull this down today.

Only issue was in : SignOutHelper in Android needs a new namespace to be brought in.

This will eventually go away though to be honest as the signout should just bring you back to the sign in page :)

After that, I approve!

jamesmontemagno · 2017-09-26T20:26:26Z

Guess who has permissions to this repo now!!!! <3 I think I figure this stuff out

msftclas added the cla-required label Sep 22, 2017

Basic iOS look and feel update

ab8c79d

msftclas added cla-signed and removed cla-required labels Sep 22, 2017

jamesmontemagno suggested changes Sep 22, 2017

View reviewed changes

sthewissen added 2 commits September 22, 2017 19:47

Removed obsolete App singleton

e65dff7

Updated images to match guidelines

e8f8e9a

- Renamed existing images - Trimmed away a lot of images - Moved stuff into asset catalogs

sthewissen added 2 commits September 22, 2017 21:28

Removed some more images

4a841f7

Removed and standardized images across all projects

88f2059

Minor fix for toolbaritems in Windows

861ff07

sthewissen mentioned this pull request Sep 24, 2017

[App] Update all image names + remove unused assets #17

Closed

sthewissen added 5 commits September 25, 2017 10:03

Removed some hardcoded text

49be5b2

These can simply be put in the XAML.

Removed the custom renderers for iOS for #23

a07625a

Useless comments are useless

ad05052

More useless comments

1298938

Moved interface to correct folder

5b5a715

sthewissen mentioned this pull request Sep 25, 2017

[Cleanup] Namespaces, project names etc. #24

Closed

Fixed namespaces for #24

b81468e

sthewissen changed the title ~~Basic iOS look and feel update~~ Basic project cleanup tasks Sep 25, 2017

Removed unused file

f86cf0e

microsoft deleted a comment from msftclas Sep 26, 2017

msftgits removed the cla-signed label Sep 26, 2017

jamesmontemagno changed the base branch from master to pr_review September 26, 2017 20:25

jamesmontemagno approved these changes Sep 26, 2017

View reviewed changes

Added a namespace in SignoutHelper that was missing

595202c

jamesmontemagno changed the base branch from pr_review to master September 26, 2017 20:26

jamesmontemagno changed the base branch from master to dev September 26, 2017 20:31

jamesmontemagno merged commit 6bb023a into microsoft:dev Sep 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic project cleanup tasks #15

Basic project cleanup tasks #15

sthewissen commented Sep 22, 2017

jamesmontemagno commented Sep 22, 2017

jamesmontemagno Sep 22, 2017

sthewissen Sep 22, 2017

jamesmontemagno Sep 22, 2017

jamesmontemagno Sep 22, 2017

jamesmontemagno Sep 22, 2017

sthewissen Sep 22, 2017

jamesmontemagno Sep 22, 2017

sthewissen Sep 22, 2017

jamesmontemagno Sep 22, 2017

edsnider Sep 22, 2017 •

edited

Loading

sthewissen Sep 22, 2017

jamesmontemagno Sep 25, 2017

jamesmontemagno commented Sep 22, 2017

sthewissen commented Sep 22, 2017 •

edited

Loading

jamesmontemagno commented Sep 22, 2017

sthewissen commented Sep 22, 2017

jamesmontemagno commented Sep 23, 2017

jamesmontemagno commented Sep 26, 2017 •

edited

Loading

jamesmontemagno commented Sep 26, 2017

		@@ -19,6 +19,8 @@ public partial class App : Application

		#region Public Fields

		public static App Instance;

Basic project cleanup tasks #15

Basic project cleanup tasks #15

Conversation

sthewissen commented Sep 22, 2017

jamesmontemagno commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsnider Sep 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesmontemagno commented Sep 22, 2017

sthewissen commented Sep 22, 2017 • edited Loading

jamesmontemagno commented Sep 22, 2017

sthewissen commented Sep 22, 2017

jamesmontemagno commented Sep 23, 2017

jamesmontemagno commented Sep 26, 2017 • edited Loading

jamesmontemagno commented Sep 26, 2017

edsnider Sep 22, 2017 •

edited

Loading

sthewissen commented Sep 22, 2017 •

edited

Loading

jamesmontemagno commented Sep 26, 2017 •

edited

Loading