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

Add a basic Options modal #10215

Merged
merged 14 commits into from Oct 17, 2018

Conversation

@noisysocks
Member

noisysocks commented Sep 27, 2018

Implements half of #10210.

Adds a basic Options modal:

localhost_8888_wp-admin_post-new php screen recording 2

Not included in this PR is the ability to toggle "advanced panels" AKA metaboxes.

To test:

  1. Open the editor
  2. Open the More drop-down menu in the top right of Gutenberg
  3. Select Options
  4. Check that each setting works and that they're persisted when the page reloads

@noisysocks noisysocks force-pushed the add/options-modal branch 2 times, most recently from c7c3e1f to 77f702c Sep 30, 2018

@noisysocks noisysocks requested a review from talldan Sep 30, 2018

}
return state;
},
panels( state = PREFERENCES_DEFAULTS.panels, action ) {

This comment has been minimized.

@noisysocks

noisysocks Sep 30, 2018

Member

Wondering if I should modify the existing panels state tree for our needs e.g. panels: { 'post-status': { enabled: true, shown: false } } instead of adding a whole new disabledPanels key.

And, if so, is it worth writing migration logic so that the shown/hidden panel state isn't lost when users update Gutenberg?

@talldan

This comment has been minimized.

@noisysocks

noisysocks Oct 4, 2018

Member

Think I'll merge disabledPanels and panels. It doesn't matter that users upgrading Gutenberg will have to re-open a few panels here and there.

@noisysocks noisysocks requested a review from gziolo Sep 30, 2018

@talldan

This comment has been minimized.

Contributor

talldan commented Sep 30, 2018

Hey - had a test of this. Looks nice. 👍

When toggling tips you I saw this, which looks a bit weird. I wonder whether the modal should be elevated over the tip. Fun with z-indexes 😄

screen shot 2018-09-30 at 3 45 03 pm

Confusingly, clicking outside of the tip dimisses the modal, even though the tip is on top.

Also, separately, the blank space at the bottom of the modal seems like it shouldn't be there. I imagine you'd only see that on larger screen sizes:

screen shot 2018-09-30 at 3 59 31 pm

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 4, 2018

When toggling tips you I saw this, which looks a bit weird. I wonder whether the modal should be elevated over the tip. Fun with z-indexes 😄

Hmm, but then what happens when we inevitably want to have a tip point to something in a modal? I might look at saving the Tips on/off setting when the modal is dismissed.

Also, separately, the blank space at the bottom of the modal seems like it shouldn't be there. I imagine you'd only see that on larger screen sizes:

👌 will look into this! Thanks for the review.

@talldan

This comment has been minimized.

Contributor

talldan commented Oct 5, 2018

Hmm, but then what happens when we inevitably want to have a tip point to something in a modal?

I also thought that, but then thought probably not worth worrying about until it happens.

I might look at saving the Tips on/off setting when the modal is dismissed.

That might be a better user experience generally - definitely less noisy. Just for tips though or all options? Only drawback would be if the user tries to navigate away with unsaved changes to the checkboxes.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 8, 2018

@noisysocks ping me when you are ready with the updates around making data layer API more general we discussed last week 🙇

@noisysocks noisysocks force-pushed the add/options-modal branch from 77f702c to 7fcbf7c Oct 11, 2018

@noisysocks noisysocks force-pushed the add/options-modal branch 2 times, most recently from 36f037e to 2645689 Oct 12, 2018

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 12, 2018

Hi! 👋

@talldan: I've rebased this and updated it to handle custom taxonomies and set the 'Show Tips' setting when the modal is dismissed. Would you mind giving it another comprehensive review?

@gziolo: I did not add an API that lets developers disable block inspector panels to this PR. I'd rather that we tackled this in a future PR so that we can give that new code the attention that it deserves. Still, if there's anything here that you think should change to make our lives easier in the future—let me know about it!

@noisysocks noisysocks force-pushed the add/options-modal branch 3 times, most recently from 1c8e6e6 to 5308f95 Oct 12, 2018

@talldan

@noisysocks - This looks really good! In terms of UX, I only have a concern about the inconsistency with some options enabling immediately and others being deferred, but lets proceed see if anyone mentions it (probably unlikely).

I think I might have spotted a couple of things around deprecations, but let me know if I'm wrong.

Show resolved Hide resolved packages/edit-post/src/store/selectors.js Outdated
}
}
export const EnablePublishSidebarOption = compose(

This comment has been minimized.

@talldan

talldan Oct 12, 2018

Contributor

This file is very compositiony! Really nice use of composition. 😄

This comment has been minimized.

@noisysocks

noisysocks Oct 16, 2018

Member

Yeah I'm pretty pleased with this!

Show resolved Hide resolved packages/edit-post/src/store/actions.js
@gziolo

I hope to do more in-depth review on Monday. However, don't wait for me if you think it's ready to go. We already pair-reviewed most of the code so this is definitely aligned with what I envisioned for this feature.

Show resolved Hide resolved packages/edit-post/src/store/actions.js
case 'TOGGLE_PANEL_OPENED': {
const { panelName } = action;
const isOpen = state[ panelName ] === true || get( state, [ panelName, 'opened' ], false );

This comment has been minimized.

@gziolo

gziolo Oct 12, 2018

Member

Backward compatible 💯

This comment has been minimized.

@noisysocks
Show resolved Hide resolved packages/edit-post/src/store/selectors.js Outdated
Show resolved Hide resolved test/e2e/specs/nux.test.js Outdated
Show resolved Hide resolved packages/edit-post/src/components/options-modal/index.js

@gziolo gziolo added this to the 4.1 milestone Oct 12, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 12, 2018

I did not add an API that lets developers disable block inspector panels to this PR. I'd rather that we tackled this in a future PR so that we can give that new code the attention that it deserves.

I didn't expect that, I think your data related changes look solid, it should make this earlier in the future. I think that inspector panels won't be customizable until we start phase 2. It's too much work. Let's focus on backward compatibility.

@gziolo gziolo requested review from jasmussen and WordPress/gutenberg-core Oct 12, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 15, 2018

I noticed 3 design issues which should be further discussed:

  1. When all document options are disabled, it renders empty content, should we add a button which allows to enable some of the panels through the modal?
    screen shot 2018-10-15 at 11 21 51
  2. Options were moved to their own group which doesn't have a title, should it remain in one of the existing groups instead?
    screen shot 2018-10-15 at 11 22 25
  3. You can toggle options by clicking checkbox or text, in my opinion we should expand it to the full width of the row
    screen shot 2018-10-15 at 11 23 56

@noisysocks noisysocks force-pushed the add/options-modal branch from 1169eaa to fed61bb Oct 16, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

I see you have already fixed the issue with Status & Visibility which should always be rendered.

I see that it's possible to hide Author fiel. @mtias should we also add it to the modal here?

screen shot 2018-10-16 at 11 48 46

@gziolo

gziolo approved these changes Oct 16, 2018

All my feedback was addressed. It works as expected. There is still this open discussion about doing refactor for panel names and their components: https://github.com/WordPress/gutenberg/pull/10215/files#r225102139. However it isn't a blocker in my opinion. We can handle it as follow-up when we agree how to make it more conistent.

There is still this issue @afercia raised which isn't introduced by this PR. It existed before with the Keyboard shortcuts modal dialog, as well with sidebars as filed in #1918. I personally believe, that we should find a proper way of handling this issue globally rather than trying to patch it with some hardcoded focus return in this PR. I added comment #1918 (comment) to ensure it's taken into account when #1918 is solved.

@mtias

This comment has been minimized.

Contributor

mtias commented Oct 16, 2018

I personally believe, that we should find a proper way of handling this issue globally rather than trying to patch it with some hardcoded focus return in this PR

Should definitely be handled globally as we cannot assume plugin / block authors would adhere to it consistently. I propose keeping track of a fallbackFocus reference across the app.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 16, 2018

I agree; the issue of focus is not specific to this PR and should not hold it up.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

Good job @aduth, your check works, Travis fails because:

There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!

It took me a while to figure out because we run everything concurrently.

@aduth

This comment has been minimized.

Member

aduth commented Oct 16, 2018

It took me a while to figure out because we run everything concurrently.

Yeah, was raised in #core-restapi as well yesterday. See #10625.

@afercia

This comment has been minimized.

Contributor

afercia commented Oct 16, 2018

I agree; the issue of focus is not specific to this PR and should not hold it up.

@tofumatt has a specific issue been opened? If so, can you please me point me at it?
Just noticed it happens also when opening the "Keyboard Shortcuts" modal from the More menu:

screen shot 2018-10-16 at 21 57 40

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 16, 2018

It took me a while to figure out because we run everything concurrently.

It's pretty hard to spot the error among the noise—thanks for the pointer! 😅

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 16, 2018

@tofumatt has a specific issue been opened? If so, can you please me point me at it?

It's the same bug as described in #1918. @gziolo and I have noted that there are some additional action items to take over in the comments there.

@noisysocks noisysocks merged commit 0434f9b into master Oct 17, 2018

2 checks passed

codecov/project 49.51% (-0.01%) compared to 6e9c18f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the add/options-modal branch Oct 17, 2018

@noisysocks noisysocks referenced this pull request Oct 17, 2018

Merged

Add Custom Fields and Advanced Panels to Options modal #10676

3 of 3 tasks complete
@StaggerLeee

This comment has been minimized.

StaggerLeee commented Oct 22, 2018

Why not add in Options modal an option for resizing editor (content) width ? With live preview.
Some number slider, or number field.

You would make life easier for all non coders who install finished themes. Not a small number, you have to agree.

I personaly each time I go to the Gutenberg to test new things think, damn how narrow is editor content width. I literally stare in huge empty white space.

Someone here on Github mentioned survey, and how this width is most natural for majority of Users. I would like to see results of this survey, not to blindly trust anyone's statement.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Oct 23, 2018

@StaggerLeee: We won't be adding any more features to Gutenberg so that we can focus on polishing what we have for WordPress 5.0. I'd encourage you to open up a new issue with your feature suggestion and we can look into it for a future release 😄

@StaggerLeee

This comment has been minimized.

StaggerLeee commented Oct 23, 2018

If you promise me not to close it directly. I am tired of playing Pinata. 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment