wordpress-mobile / WordPress-Editor-iOS

⛔️ [DEPRECATED] A reusable iOS rich text editor component.
GNU General Public License v2.0
1.06k stars 210 forks source link

Refresh legacy editor #801

Closed SergioEstevao closed 8 years ago

SergioEstevao commented 8 years ago

Update the legacy editor by copying the design for the format bar and actions from the visual editor.

This PR also includes a lot of cleaning up of custom controls for the format bar and replacing them by a standard UIToolbar and UIBarButtonItem. This make the code work a lot nicer on the iPad using multitasking mode and on the iPhone 6s.

screen shot 2016-04-11 at 13 36 17

There is this branch on the main app editor/test_new_legacy_editor to test the integration of this new version.

Needs Review: @bummytime and @astralbodies

astralbodies commented 8 years ago

I am seeing this warning: 2016-04-12_07-35-58

astralbodies commented 8 years ago

I think the styling of the keyboard accessory bar should match the regular editor.

In WPiOS, the coloring looks way off: 2016-04-12_07-37-13

versus

2016-04-12_07-38-28

Comparison of EditorDemo:

2016-04-12_07-40-21

astralbodies commented 8 years ago

I'll leave the ultimate decision about design up to @bummytime with his amount of experience redesigning the visual editor.

If we're going to support media with this PR I'd like to see some sort of visual cue that an upload is happening that is cancel-able and also accessible with VoiceOver.

SergioEstevao commented 8 years ago

@astralbodies we do support media in the legacy editor, but just images, and we show a progress indicator in the navigation bar where you can click and see the progress. screen shot 2016-04-12 at 14 31 55screen shot 2016-04-12 at 14 31 59

astralbodies commented 8 years ago

@SergioEstevao Okay I wasn't seeing that because I was in text editing mode with the keyboard visible.

SergioEstevao commented 8 years ago

@astralbodies the media experience that was is one the reasons that I wanted to improve the legacy editor. And the WPMediaPicker is accessible i think I do the same description that the photos app does but I can improve on that.

bummytime commented 8 years ago

Nice work on this @SergioEstevao! So looking at the editor demo, 3 things strike me about the updated legacy editor toolbar:

1) All of the buttons feel like they are always selected or on. I am wondering if we shouldn't default to the grey color used in the other toolbar and then have the WordPress blue come in when a user taps a button and revert back to the same grey. When working with previous designers on the new toolbar, it was a design goal to make sure the content takes priority when typing, not the toolbar. Seeing the blue buttons focuses my eye there. I understand the user will not be using the editors side-by-side to make such comparisons, but I think it may be something to consider:

scaled screen shot 2016-04-12 at 10 01 23 am

2) The toolbar button spacing seems unbalanced when in portrait mode on the 6 and 6 Plus. I know they are in groupings, which makes sense in landscape or bigger screens, I get a sense they are misaligned when pushed together more closely:

scaled screen shot 2016-04-12 at 10 13 05 am

3) On iPads, the icons and their groupings seem like they could be spread out a little bit more. Lots of whitespace:

iPad Pro:

scaled screen shot 2016-04-12 at 10 20 39 amscaled screen shot 2016-04-12 at 10 20 51 am

iPad Retina:

scaled screen shot 2016-04-12 at 10 21 43 amscaled screen shot 2016-04-12 at 10 21 51 am

As always, my disclaimer is that I am not a designer...I just play one on TV.

bummytime commented 8 years ago

Another think I noticed @SergioEstevao is that when testing multitasking on the iPad pro, i was able to get the format bar to "stick" multiple times when resizing and rotating. This may be a bug for another PR, but wanted to note it here:

screen shot 2016-04-12 at 10 23 18 am

I had a LOT of fun with the same issue here: https://github.com/wordpress-mobile/WordPress-Editor-iOS/issues/736 :grin:

SergioEstevao commented 8 years ago

@bummytime do you mind to have a look again for the toolbar stuck/size issues? I think I've got them sorted out with this last commit.

SergioEstevao commented 8 years ago

I created this branch on the main app editor/test_new_legacy_editor to help test the integration of this new version.

bummytime commented 8 years ago

@SergioEstevao the toolbar is looking better! It no longer gets stuck while multitasking, colors are more inline with the other toolbar, and the spacing between icons looks good on the 6 and 6 Plus:

scaled screen shot 2016-04-13 at 8 25 25 pm scaled screen shot 2016-04-13 at 8 25 00 pm

Did you want to have different background grays on the legacy and "new" toolbars? (I realize the appearance proxy settings in your WPiOS test branch may not be final.)

SergioEstevao commented 8 years ago

@bummytime ready for another look, I also update the branch on the main app for testing. You can also have look in the code now, because I think it's final now.

bummytime commented 8 years ago

@SergioEstevao This is really coming together nice! There is 1 more visual nit to look at:

napkin 28 04-15-16 10 45 06 am

I think there needs to be a little more upper & lower padding on the post title and more upper padding on the content body. It seems a little cramped (especially on an iPad Pro :grinning: ).

bummytime commented 8 years ago

@SergioEstevao One other question for you...do you think we should make the font color black as well?

bummytime commented 8 years ago

@SergioEstevao Code-wise I think you are in good shape — love all that red! Let me know your thoughts on the two questions above. Nice work!

SergioEstevao commented 8 years ago

@bummytime I followed your lead and updated the size of the title field and switch the color to black.

bummytime commented 8 years ago

Awesome sauce. SO I am going to give you a :shipit: and call out one thing you may or may not want to address:

napkin 29 04-15-16 1 37 37 pm

You may want a little more breathing room (padding) above the content body. Do what you will. :+1: :v:

SergioEstevao commented 8 years ago

I increased the space as you asked @bummytime. You can see it here:

screen shot 2016-04-15 at 22 38 38

I'm going forward and merge this in, thanks for the thorough review!

bummytime commented 8 years ago

:boom: