Fix submit for review button by oandregal · Pull Request #10941 · WordPress/gutenberg

oandregal · 2018-10-23T10:32:29Z

This PR takes the viewport size into account when it comes to decide whether we show the button or toggle logic. Here's how this PR modifies the logic (changes in bold):

Logic	Before	After
Post status is `publish` or `private`	Button	Button
Post status is `future` and the (scheduled) date is after now	Button	Button
Post status is `pending`, cannot be published and the viewport is < medium	Button	Toggle
Post status is `pending`, cannot be published and the viewport >= medium	Button	Button
Publish sidebar is enabled	Toggle	Toggle
Publish sidebar is disabled and viewport is < medium	Button	Toggle
Publish sidebar is disabled and viewport is >= medium	Button	Button

TODO

Show the "Publish..." button below medium viewports overriding the pre-publish checks option if necessary.
Hide pre-publish checks option on the general settings modal below medium viewports.
Update/Add e2e tests.

jasmussen · 2018-10-23T10:56:03Z

Hmm am I doing something wrong?

I have unchecked prepublish checks, but they're still not present on mobile:

oandregal · 2018-10-23T11:04:54Z

Hmm am I doing something wrong?

Testing before this is ready! 😄

I'm still working on this. I should have waited before pinging you as a reviewer.

jasmussen · 2018-10-23T11:06:27Z

No worries. I'll keep an eye on this!

oandregal · 2018-10-23T12:19:10Z

@jasmussen @WordPress/gutenberg-core this is ready for review. I still need to get ready the e2e tests, but wanted to give folks as much time as possible for review. Need to be away a couple of hours but will come back to this by the end of the day.

jasmussen · 2018-10-23T12:24:50Z

This is excellent. I can confirm this works as intended 👍 👍

This should go right in once the e2e tests are fixed. Am I correct in thinking this replaces #10803? If so we should thank @nfmohit-wpmudev for his work so far, and his inspiration for this.

oandregal · 2018-10-23T15:52:03Z

Added some basic e2e tests in c7b89a2.

noisysocks

This works in my testing and is looking really good! Great work 💯📈🎉

I left a few notes on how I think we could make the code a little easier to understand.

oandregal · 2018-10-24T11:15:30Z

@noisysocks done a refactoring to clarify the button or toggle logic, as you suggested. Also added a bunch of unit tests. Everything is much clearer now! Added a table to the PR description with the changes and some code comments to add further context to our futureselves.

oandregal · 2018-10-24T11:23:16Z

This was a leftover, wasn't being used.

I've noticed we're selecting stuff that we don't need all over the place. I wonder if we could somehow write a lint rule to catch them all...

Some languages such as Deustch have longer text that need acomodation. We are also using medium to hide the writing modes, so it makes sense to follow that.

We want to show the "Publish..." button on < medium viewports, no matter what the isPublishSidebarEnabled state is.

…check

noisysocks

Yes! Love it! This is a lot easier to follow. Thanks also for documenting the logic both in the code and in the PR. Gold star for you ⭐️

There were some minor indentation issues in post-publish-button-or-toggle.js. I fixed them up by running Prettier on the file in f4aed3d. Hope you don't mind!

oandregal · 2018-10-25T07:31:07Z

Thanks for f4aed3d, Robert!

…Press#10941)

oandregal self-assigned this Oct 23, 2018

oandregal added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Oct 23, 2018

oandregal added this to the 4.2 milestone Oct 23, 2018

oandregal mentioned this pull request Oct 23, 2018

"Submit for Review" button regression on mobile #10475

Closed

oandregal added the [Status] In Progress Tracking issues with work in progress label Oct 23, 2018

oandregal requested review from a team and jasmussen October 23, 2018 10:35

oandregal changed the title ~~Fix/submit for review~~ Fix/submit for review button on small viewports Oct 23, 2018

oandregal changed the title ~~Fix/submit for review button on small viewports~~ Fix submit for review button on small viewports Oct 23, 2018

oandregal changed the title ~~Fix submit for review button on small viewports~~ Fix submit for review button Oct 23, 2018

oandregal removed the [Status] In Progress Tracking issues with work in progress label Oct 23, 2018

noisysocks reviewed Oct 24, 2018

View reviewed changes

Comment thread packages/edit-post/src/components/header/index.js Outdated

Comment thread test/e2e/specs/publishing.test.js Outdated

Comment thread packages/edit-post/src/components/options-modal/options.js Outdated

Comment thread packages/edit-post/src/components/header/index.js Outdated

oandregal commented Oct 24, 2018

View reviewed changes

oandregal added 9 commits October 24, 2018 13:48

Change check order

f38268f

Add isPublished check to header

3daf525

Add scheduling check to header

6558f3c

Add publish action check to header

5f4319b

Remove unnecessary code

3d841d2

Force the Publish button on small viewports

9c567eb

Hide for viewports < medium

6a06dee

Some languages such as Deustch have longer text that need acomodation. We are also using medium to hide the writing modes, so it makes sense to follow that.

Rename variable for clarity

eea3b15

Override the isPublishSidebarEnabled option in < medium viewports

164bf6f

We want to show the "Publish..." button on < medium viewports, no matter what the isPublishSidebarEnabled state is.

oandregal added 17 commits October 24, 2018 13:48

Hide pre-publish check options in < medium viewports

973757e

Add e2e test to check buttons

d149c7b

Update snapshot

f050663

Add comment

ebc22dd

Extract PostPublishButtonOrToggle component

81c2d6d

More refactoring

f375607

Test that button is rendered when post is published

aa2e271

Tests for publish sidebar not enabled

2455a0b

Tests for post scheduled

275209b

Tests for post pending that cannot be published

31fadab

Better describe tests

d5be495

Better describe test

24f7db5

Refactor conditions to show toggle or button

b4abb46

Add back the isPending and cannot be published for > small viewports …

9c749a2

…check

Add test for is pending status

21cbc8b

Rename variables and add tests

b90fb2b

Bring back a property deleted by accident

b94538d

oandregal force-pushed the fix/submit-for-review branch from 53615dd to b94538d Compare October 24, 2018 11:48

Format post-publish-button-or-toggle.js using prettier

f4aed3d

noisysocks approved these changes Oct 25, 2018

View reviewed changes

oandregal merged commit 8ce47cb into master Oct 25, 2018

oandregal deleted the fix/submit-for-review branch October 25, 2018 07:31

oandregal mentioned this pull request Oct 25, 2018

[Fix] Make disabled state of 'Pre-publish checks' affect desktop viewport only #10803

Closed

4 tasks

antpb pushed a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Include viewport size in logic to show publish toggle or button (Word…

6bd27d0

…Press#10941)

Uh oh!

Conversation

oandregal commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

jasmussen commented Oct 23, 2018

Uh oh!

oandregal commented Oct 23, 2018

Uh oh!

jasmussen commented Oct 23, 2018

Uh oh!

oandregal commented Oct 23, 2018

Uh oh!

jasmussen commented Oct 23, 2018

Uh oh!

oandregal commented Oct 23, 2018

Uh oh!

noisysocks left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oandregal commented Oct 24, 2018

Uh oh!

oandregal Oct 24, 2018

Choose a reason for hiding this comment

Uh oh!

noisysocks Oct 25, 2018

Choose a reason for hiding this comment

Uh oh!

noisysocks left a comment

Choose a reason for hiding this comment

Uh oh!

oandregal commented Oct 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oandregal commented Oct 23, 2018 •

edited

Loading