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

Chrome: Use an expandable panel for the block's advanced settings #2857

Merged
merged 2 commits into from Oct 3, 2017

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Oct 2, 2017

Description

closes #2851

Screenshots (jpeg or gifs if applicable):

screen shot 2017-10-02 at 14 09 10

@youknowriad youknowriad added the Chrome label Oct 2, 2017

@youknowriad youknowriad self-assigned this Oct 2, 2017

@youknowriad youknowriad requested review from mtias and jasmussen Oct 2, 2017

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Oct 2, 2017

Contributor

@jasmussen should we call this one advanced? Looping back to our early mockups here :)

Contributor

mtias commented Oct 2, 2017

@jasmussen should we call this one advanced? Looping back to our early mockups here :)

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 2, 2017

Contributor

Yes, I think that makes sense, otherwise it's "Block" all over.

Contributor

jasmussen commented Oct 2, 2017

Yes, I think that makes sense, otherwise it's "Block" all over.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 3, 2017

Contributor

@jasmussen so just "Advanced" ? or more like "Advanced Settings"?

Contributor

youknowriad commented Oct 3, 2017

@jasmussen so just "Advanced" ? or more like "Advanced Settings"?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 3, 2017

Contributor

I'm okay with either "Advanced" or "Advanced Settings". I like Advanced as it's a bit shorter, and ideally we'll only put stuff in here that really you'd only want to customize once in a blue moon.

Contributor

jasmussen commented Oct 3, 2017

I'm okay with either "Advanced" or "Advanced Settings". I like Advanced as it's a bit shorter, and ideally we'll only put stuff in here that really you'd only want to customize once in a blue moon.

@gziolo

gziolo approved these changes Oct 3, 2017

I think it can land as soon as text is updated and margin-bottom is confirmed. It works as expected.

@@ -34,6 +34,10 @@
}
}
.editor-block-inspector__content .editor-advanced-controls {
margin-bottom: -14px;

This comment has been minimized.

@gziolo

gziolo Oct 3, 2017

Member

Why not -16px like in other places? There is still a small whitespace added with the current value:

screen shot 2017-10-03 at 11 58 06

@gziolo

gziolo Oct 3, 2017

Member

Why not -16px like in other places? There is still a small whitespace added with the current value:

screen shot 2017-10-03 at 11 58 06

This comment has been minimized.

@youknowriad

youknowriad Oct 3, 2017

Contributor

Because of the focus style when clicked, I'm fine with both, but seems better to leave the focus style visible when closed.

@youknowriad

youknowriad Oct 3, 2017

Contributor

Because of the focus style when clicked, I'm fine with both, but seems better to leave the focus style visible when closed.

This comment has been minimized.

@gziolo

gziolo Oct 3, 2017

Member

@jasmussen should have a better idea what's best. I'm only asking to learn myself :)

@gziolo

gziolo Oct 3, 2017

Member

@jasmussen should have a better idea what's best. I'm only asking to learn myself :)

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 3, 2017

Codecov Report

Merging #2857 into master will increase coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2857      +/-   ##
=========================================
+ Coverage   33.78%   33.9%   +0.11%     
=========================================
  Files         191     191              
  Lines        5692    5699       +7     
  Branches      997    1000       +3     
=========================================
+ Hits         1923    1932       +9     
+ Misses       3189    3187       -2     
  Partials      580     580
Impacted Files Coverage Δ
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

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 d420547...e0e4e74. Read the comment docs.

codecov bot commented Oct 3, 2017

Codecov Report

Merging #2857 into master will increase coverage by 0.11%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2857      +/-   ##
=========================================
+ Coverage   33.78%   33.9%   +0.11%     
=========================================
  Files         191     191              
  Lines        5692    5699       +7     
  Branches      997    1000       +3     
=========================================
+ Hits         1923    1932       +9     
+ Misses       3189    3187       -2     
  Partials      580     580
Impacted Files Coverage Δ
...ditor/sidebar/block-inspector/advanced-controls.js 0% <0%> (ø) ⬆️
blocks/api/serializer.js 98.14% <0%> (+0.23%) ⬆️
blocks/api/paste/index.js 86.66% <0%> (+7.35%) ⬆️

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 d420547...e0e4e74. Read the comment docs.

@youknowriad youknowriad merged commit 431ad99 into master Oct 3, 2017

3 checks passed

codecov/project 33.9% (+0.11%) compared to d420547
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/advanced-block-settings-panel branch Oct 3, 2017

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