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

Fix auto-hiding appender regression. #20780

Merged
merged 1 commit into from
Mar 11, 2020
Merged

Conversation

jasmussen
Copy link
Contributor

In #20753 I caused a regression that collapsed the big square appender that exists in an empty columns block, or an empty group block.

The only thing that should collapse, until selected, is the black appender button inside blocks like Social Links, Buttons, or Navigation Menu.

This PR fixes that regression by scoping the animation to said button.

appender

In #20753 I caused a regression that collapsed the big square appender that exists in an empty columns block, or an empty group block.

The only thing that should collapse, until selected, is the black appender button inside blocks like Social Links, Buttons, or Navigation Menu.

This PR fixes that regression by scoping the animation to said button.
@jasmussen jasmussen added [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release labels Mar 11, 2020
@jasmussen jasmussen self-assigned this Mar 11, 2020
@github-actions
Copy link

Size Change: +4 B (0%)

Total Size: 864 kB

Filename Size Change
build/block-editor/style-rtl.css 10.6 kB +4 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 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 105 kB 0 B
build/block-editor/style.css 10.6 kB 0 B
build/block-library/editor-rtl.css 7.25 kB 0 B
build/block-library/editor.css 7.25 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.38 kB 0 B
build/block-library/style.css 7.39 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 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 191 kB 0 B
build/components/style-rtl.css 15.5 kB 0 B
build/components/style.css 15.5 kB 0 B
build/compose/index.js 5.75 kB 0 B
build/core-data/index.js 10.6 kB 0 B
build/data-controls/index.js 1.03 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 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.06 kB 0 B
build/edit-post/index.js 91.3 kB 0 B
build/edit-post/style-rtl.css 8.64 kB 0 B
build/edit-post/style.css 8.64 kB 0 B
build/edit-site/index.js 4.64 kB 0 B
build/edit-site/style-rtl.css 2.48 kB 0 B
build/edit-site/style.css 2.48 kB 0 B
build/edit-widgets/index.js 4.44 kB 0 B
build/edit-widgets/style-rtl.css 2.58 kB 0 B
build/edit-widgets/style.css 2.58 kB 0 B
build/editor/editor-styles-rtl.css 381 B 0 B
build/editor/editor-styles.css 382 B 0 B
build/editor/index.js 43.8 kB 0 B
build/editor/style-rtl.css 3.98 kB 0 B
build/editor/style.css 3.97 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.09 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 1.92 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.49 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 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 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.01 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.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 780 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.55 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@youknowriad youknowriad requested a review from ItsJonQ March 11, 2020 09:11
@jasmussen
Copy link
Contributor Author

Hey @ItsJonQ my friend, I would appreciate your thoughts on this one. I think this PR is fairly solid as it fixes the regression I introduced, so we should consider merging it.

However you can see the reason why the regression was introduced in the first place was because of handling in CSS something that should perhaps be handled in a better way. If you have the time and energy, do you have some insights to share as to how this might be handled better?

@ItsJonQ
Copy link

ItsJonQ commented Mar 11, 2020

@jasmussen I'll take a looksy! 🤗

@jasmussen
Copy link
Contributor Author

Thanks. Should we merge this one in the mean time? It's either that, or revert my previous PR. Open to either, but I'm feeling pretty solid about this one.

The key to look for when testing: — the black plus should disappear on social links, navigation menu and buttons, and appear when selected. But the white appeander that sits in an empty group block, or an empty colums block, should always be visible.

@ItsJonQ
Copy link

ItsJonQ commented Mar 11, 2020

@jasmussen Merge it up 💪 .

I just took a quick look. In general, I think the closer we tie interaction/style logic JS (React components) the less brittle it would be. The class update you made is a great example. It feels like a very loose connection between function (component) and style (presentation). The more compounded classes there are, the looser the connection feels (at least to me) 😊

@jasmussen jasmussen merged commit 08ee3cf into master Mar 11, 2020
@jasmussen jasmussen deleted the fix/appender-regression branch March 11, 2020 16:58
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 11, 2020
@jasmussen
Copy link
Contributor Author

Thanks Q! Your assessment of the behavior is also an eloquent expression of what I sensed writing the CSS. It works, but it could be clean and more solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants