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

Mobile editor redesign #5987

Merged
merged 31 commits into from
Dec 21, 2016
Merged

Mobile editor redesign #5987

merged 31 commits into from
Dec 21, 2016

Conversation

DanReyLop
Copy link
Contributor

@DanReyLop DanReyLop commented Jun 13, 2016

This is the PR with the whole mobile editor redesign combined.

Issue: #4266

PR 1: #4456
PR 2: #4686

I've fixed all the bugs that @alisterscott discovered in #4686 , except for "Keyboard cursor keeps showing when switching to the metadata tab" because I couldn't reproduce it in Android and I don't have an iPhone at hand.

Calypso Live branch → https://calypso.live/posts?branch=update/4266-mobile-editor-redesign-2

cc/ @mtias @alisterscott @folletto @aduth

@DanReyLop DanReyLop added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 13, 2016
@DanReyLop DanReyLop self-assigned this Jun 13, 2016
@folletto
Copy link
Contributor

Tested on Chrome, Safari, Firefox, Edge, IE11: 👍

I found two tiny visual issues:

  1. The edit view misses the top 1px border.
  2. The .editor__header would benefit from a padding-top: 8px. :)

screen shot 2016-06-13 at 17 15 44

On Edge I got flashes of white: the UI went white until scrolled. I couldn't replicate reliably tho, and on scroll it went back to normal.

@rickybanister rickybanister added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jun 16, 2016
@DanReyLop DanReyLop force-pushed the update/4266-mobile-editor-redesign-2 branch from d013221 to ebef9db Compare June 18, 2016 20:12
@DanReyLop
Copy link
Contributor Author

Fixed :)

@alisterscott
Copy link
Contributor

I can't reproduce any of the previous bugs and couldn't find any more issues except for a warning in the console when running locally warning.js:44 Warning:valueprop oninputshould not be null. Consider using the empty string to clear the component orundefinedfor uncontrolled components.
So, 👍 LGTM

@folletto folletto removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jun 20, 2016
@folletto
Copy link
Contributor

All good design wise: 👍

if ( editor.theme.panel.getEl().clientHeight ) {
tinymce.DOM.setStyles(editor.getContainer(), {
'padding-top': containerPadding
});
Copy link
Contributor

Choose a reason for hiding this comment

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

@aduth
Copy link
Contributor

aduth commented Jun 22, 2016

Looking good! Needs rebase.

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 22, 2016
@DanReyLop DanReyLop force-pushed the update/4266-mobile-editor-redesign-2 branch from ebef9db to 771a90e Compare June 29, 2016 12:44
@DanReyLop
Copy link
Contributor Author

I've fixed all the comments.

The rebase with master was a MAYOR pain in the ***, mainly because of this PR: #5475

That PR assumes that the Publish button will always be in the EditorGroundControl. So it will look quite bad in mobile (the notice will point to the Choose date button instead, and the Publish button in the nav bar will be disabled for no apparent reason). I think we should fix it before merging, but I couldn't get Calypso to display that notice locally. Is there any way to get Calypso to require your account to be verified? I tried creating a new account and still didn't see the notice locally, and I could publish without verifying the e-mail.

@aduth
Copy link
Contributor

aduth commented Jun 30, 2016

cc @coreh might want to take a look or have some feedback re: reconciling with email verification notice

@DanReyLop DanReyLop added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jul 13, 2016
@DanReyLop DanReyLop force-pushed the update/4266-mobile-editor-redesign-2 branch from c46d848 to 9047ed3 Compare December 19, 2016 09:17
@DanReyLop
Copy link
Contributor Author

I've rebased and solved the merge conflicts. This PR is only missing a "good to go". The only outstanding issue is: In mobile layouts, the email verification notice is not visible anywhere, so that behaviour in mobile is the same as it was before #5475. I think we can all agree that it can be solved in a separate issue/PR.

@folletto
Copy link
Contributor

I agree to move that to a separate PR. :)

@hoverduck
Copy link
Contributor

I ran the e2e tests across this PR again, and they worked perfectly, but I noticed a visual issue at desktop width:

When an accordion drawer is opened, the sidebar header gets squished
wp-calypso-5987

@alisterscott
Copy link
Contributor

This PR is only missing a "good to go"

Good to go 🚢

I've raised @hoverduck's issue separately as #10168 as I don't believe this should hold this back

@alisterscott
Copy link
Contributor

This does make #8437 a little more prominent but that's an existing bug and not introduced in this PR so this PR shouldn't be held up due to that IMO

@DanReyLop
Copy link
Contributor Author

I created #10212 to fix the email verification notice on mobile screens.

@DanReyLop DanReyLop merged commit c654980 into master Dec 21, 2016
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 21, 2016
@DanReyLop DanReyLop deleted the update/4266-mobile-editor-redesign-2 branch December 21, 2016 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.