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

Try: Inset borders. #21498

Merged
merged 1 commit into from Apr 10, 2020
Merged

Try: Inset borders. #21498

merged 1 commit into from Apr 10, 2020

Conversation

@jasmussen
Copy link
Contributor

@jasmussen jasmussen commented Apr 9, 2020

From the department of "subtle details" comes this PR:

  • It changes the stroke around placeholders to be inset rather than outset.

And this matters more than you might think. The difference is subtle, but it boils down to this.

  • When the block focus rectangle is outset, it doesn't cover even a single pixel of content.
  • When the block focus rectangle is inset, it covers a single pixel of content.

The former benefit seems obvious, the latter less so. So why make a change? Because the latter is actually more accurate to the block footprint, and it means no borders are cropped in full-width edge situations. Let me circle back to that in a minute, but first, before:

before

After:

after

Imperceptible here, intentionally so. But what you can observe if you look closely is that the dark left border of both the block toolbar and the placeholder chrome, aligns PERFECTLY with the left margin of the paragraph block above. In the "before" image, it's actually 1px outset. At the cost of covering a pixel of text when in selection mode, of course. But there's another benefit.

One of the G2 efforts was to remove "side UI" — UI that sat left or right of the block itself, so that the block toolbar would never be cropped. This would also benefit full-wide situations. Well I'm working on a separate project where the block is full-wide, and that 1px outset border gets cropped:

Screenshot 2020-04-09 at 08 58 01

This PR fixes that:

Screenshot 2020-04-09 at 08 55 55

Previously, in the #18667 we intentionally used an outset border so as to not crop captions. However revisiting this, it doesn't feel so bad:

Screenshot 2020-04-09 at 08 54 55

This is also primarily an issue in TwentyTwenty with left-aligned captions, where in most caption situations it's a non issue, like TwentyNineteen:

Screenshot 2020-04-09 at 08 54 29

And furthermore — the covering of text is only an issue when the block has focus.

Your thoughts are welcome!

@github-actions
Copy link

@github-actions github-actions bot commented Apr 9, 2020

Size Change: +10 B (0%)

Total Size: 903 kB

Filename Size Change
build/block-editor/style-rtl.css 10.2 kB +4 B (0%)
build/block-editor/style.css 10.2 kB +2 B (0%)
build/components/style-rtl.css 16.6 kB +2 B (0%)
build/components/style.css 16.5 kB +2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-library/editor-rtl.css 7.22 kB 0 B
build/block-library/editor.css 7.22 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.15 kB 0 B
build/block-library/style.css 7.16 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 3.1 kB 0 B
build/edit-navigation/style-rtl.css 279 B 0 B
build/edit-navigation/style.css 280 B 0 B
build/edit-post/index.js 93.5 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 3.74 kB 0 B
build/edit-widgets/style.css 3.73 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.6 kB 0 B
build/editor/style-rtl.css 3.49 kB 0 B
build/editor/style.css 3.49 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.29 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.67 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.01 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@shaunandrews
Copy link
Contributor

@shaunandrews shaunandrews commented Apr 9, 2020

I can dig it. The inset border also means the border-radius matches, too.

@MichaelArestad
Copy link
Contributor

@MichaelArestad MichaelArestad commented Apr 9, 2020

This is a great subtle change! I think it actually works better than before. Well done.

@karmatosed
Copy link
Member

@karmatosed karmatosed commented Apr 9, 2020

Love it and even more so how it's a subtle improvement.

Copy link
Member

@karmatosed karmatosed left a comment

Reviewing this it looks good for me to get in. There is a build failure so won't merge yet.

@jasmussen
Copy link
Contributor Author

@jasmussen jasmussen commented Apr 10, 2020

I see thumbs up and positivity! Thanks everyone. Let's get this in and shave off that 1 pixel!

From the department of "subtle details" comes this PR.

It changes the stroke around placeholders to be _inset_ rather than _outset_.

And this matters more than you might think.
@jasmussen jasmussen force-pushed the try/inset-borders-alt branch from 75440d1 to 21cf083 Apr 10, 2020
@jasmussen jasmussen merged commit 2611a1d into master Apr 10, 2020
3 checks passed
3 checks passed
@github-actions
build
Details
@github-actions
pull-request-automation
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen deleted the try/inset-borders-alt branch Apr 10, 2020
@github-actions github-actions bot added this to the Gutenberg 7.9 milestone Apr 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants