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

Layout: Add metabox placeholder shell #2800

Merged
merged 5 commits into from Sep 26, 2017

Conversation

Projects
None yet
6 participants
@aduth
Member

aduth commented Sep 26, 2017

Closes #2760
Closes #2322
Related: #2583

Closed Opened
Closed Opened

While development of metaboxes continues in #2583, this pull request seeks to add a static content shell for representing the container in which meta fields will be rendered. In doing so, it tackles styling issues with the current layout where content does not occupy the full height of the viewport, complicating panels pinned to the bottom, and disrupting "click outside" behavior for the currently selected block.

Testing instructions:

  1. Navigate to Gutenberg > Demo or Gutenberg > New Post
  2. Note that an "Extended Settings" toggle heading is shown at the bottom of the page
  3. Toggle the Extended Settings panel
  4. Note that the panel expands upward, and that it is still possible to reach all content of the post while the panel is expanded

aduth added some commits Sep 26, 2017

Button: Improve styling consistency for Safari
Safari has default margins and resets color in active state
Panel: Set margin as flush with bounds of button
Previously there was a 1px gap, presumably specifically targetting the sidebar to allow focus outline to show on all sides, but this was problematic because (a) focus glow isn't 1px in most browsers and (b) this should be shown by allowing overflow ideally and (c) if necessary, should target only cases where it's required (sidebar)
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 26, 2017

Codecov Report

Merging #2800 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
- Coverage   33.82%   33.73%   -0.09%     
==========================================
  Files         189      190       +1     
  Lines        5661     5676      +15     
  Branches      988      991       +3     
==========================================
  Hits         1915     1915              
- Misses       3171     3183      +12     
- Partials      575      578       +3
Impacted Files Coverage Δ
editor/meta-boxes/index.js 0% <0%> (ø)
editor/layout/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c2d2bd...43b84a5. Read the comment docs.

codecov bot commented Sep 26, 2017

Codecov Report

Merging #2800 into master will decrease coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2800      +/-   ##
==========================================
- Coverage   33.82%   33.73%   -0.09%     
==========================================
  Files         189      190       +1     
  Lines        5661     5676      +15     
  Branches      988      991       +3     
==========================================
  Hits         1915     1915              
- Misses       3171     3183      +12     
- Partials      575      578       +3
Impacted Files Coverage Δ
editor/meta-boxes/index.js 0% <0%> (ø)
editor/layout/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c2d2bd...43b84a5. Read the comment docs.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 26, 2017

Contributor

🚢 it

Contributor

mtias commented Sep 26, 2017

🚢 it

@aduth aduth merged commit 23f8f05 into master Sep 26, 2017

3 checks passed

codecov/project 33.73% (-0.09%) compared to 9c2d2bd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the add/metaboxes-placeholder branch Sep 26, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 27, 2017

Contributor

💥

Contributor

jasmussen commented Sep 27, 2017

💥

@rosswintle

This comment has been minimized.

Show comment
Hide comment
@rosswintle

rosswintle Sep 27, 2017

Contributor

Not sure if this is the right place to comment. And apologies if this is all covered elsewhere, but I wanted to give some feedback on seeing this:

a) Meta boxes currently have several places that they can live: in the sidebar, below post content with different priorities and contexts
b) I also have cases where I've improved the editing experience for my users by having meta boxes above or below the title because this fits with their content editing flow.
c) I really don't like the "Extended settings" title. For some editing workflows the information in meta boxes is actually critical, core content/settings, not something optional/added-on/extended. Is this editable? Can developers add additional sections of their own like this?

Contributor

rosswintle commented Sep 27, 2017

Not sure if this is the right place to comment. And apologies if this is all covered elsewhere, but I wanted to give some feedback on seeing this:

a) Meta boxes currently have several places that they can live: in the sidebar, below post content with different priorities and contexts
b) I also have cases where I've improved the editing experience for my users by having meta boxes above or below the title because this fits with their content editing flow.
c) I really don't like the "Extended settings" title. For some editing workflows the information in meta boxes is actually critical, core content/settings, not something optional/added-on/extended. Is this editable? Can developers add additional sections of their own like this?

@mrwweb

This comment has been minimized.

Show comment
Hide comment
@mrwweb

mrwweb Sep 27, 2017

+1 for everything @rosswintle is saying. I'll add that plenty of interfaces have required meta fields which almost certainly shouldn't be kept in a collapsed-by-default area.

mrwweb commented Sep 27, 2017

+1 for everything @rosswintle is saying. I'll add that plenty of interfaces have required meta fields which almost certainly shouldn't be kept in a collapsed-by-default area.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 27, 2017

Contributor

@rosswintle @mrwweb These concerns are legitimate and have already been raised in #952

For the first iteration, we'll probably keep the collapsed state but have multiple areas.
There are some good design proposals dropping the expanding area (for the content area) and replacing them with "separators" might be good as v2 #952 (comment)

Feel free to discuss this more in the metaboxes issue

Contributor

youknowriad commented Sep 27, 2017

@rosswintle @mrwweb These concerns are legitimate and have already been raised in #952

For the first iteration, we'll probably keep the collapsed state but have multiple areas.
There are some good design proposals dropping the expanding area (for the content area) and replacing them with "separators" might be good as v2 #952 (comment)

Feel free to discuss this more in the metaboxes issue

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