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

Revert "Use UnitControl instead of RangeControl for column width" #26056

Closed
wants to merge 1 commit into from

Conversation

ceyhun
Copy link
Member

@ceyhun ceyhun commented Oct 13, 2020

Description

This temporarily reverts this PR: #24711 until regressions on web and mobile described in that PR are resolved.

@github-actions
Copy link

Size Change: -138 B (0%)

Total Size: 1.19 MB

Filename Size Change
build/block-library/index.js 145 kB -103 B (0%)
build/components/index.js 169 kB -35 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.14 kB 0 B
build/annotations/index.js 3.52 kB 0 B
build/api-fetch/index.js 3.35 kB 0 B
build/autop/index.js 2.72 kB 0 B
build/blob/index.js 667 B 0 B
build/block-directory/index.js 8.56 kB 0 B
build/block-directory/style-rtl.css 943 B 0 B
build/block-directory/style.css 942 B 0 B
build/block-editor/index.js 130 kB 0 B
build/block-editor/style-rtl.css 10.9 kB 0 B
build/block-editor/style.css 10.9 kB 0 B
build/block-library/editor-rtl.css 8.65 kB 0 B
build/block-library/editor.css 8.65 kB 0 B
build/block-library/style-rtl.css 7.7 kB 0 B
build/block-library/style.css 7.7 kB 0 B
build/block-library/theme-rtl.css 741 B 0 B
build/block-library/theme.css 741 B 0 B
build/block-serialization-default-parser/index.js 1.78 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 47.6 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 9.63 kB 0 B
build/core-data/index.js 12 kB 0 B
build/data-controls/index.js 685 B 0 B
build/data/index.js 8.63 kB 0 B
build/date/index.js 31.9 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 4.42 kB 0 B
build/edit-navigation/index.js 10.6 kB 0 B
build/edit-navigation/style-rtl.css 868 B 0 B
build/edit-navigation/style.css 871 B 0 B
build/edit-post/index.js 306 kB 0 B
build/edit-post/style-rtl.css 6.29 kB 0 B
build/edit-post/style.css 6.28 kB 0 B
build/edit-site/index.js 21.3 kB 0 B
build/edit-site/style-rtl.css 3.85 kB 0 B
build/edit-site/style.css 3.85 kB 0 B
build/edit-widgets/index.js 21.3 kB 0 B
build/edit-widgets/style-rtl.css 2.94 kB 0 B
build/edit-widgets/style.css 2.95 kB 0 B
build/editor/editor-styles-rtl.css 480 B 0 B
build/editor/editor-styles.css 482 B 0 B
build/editor/index.js 45.4 kB 0 B
build/editor/style-rtl.css 3.85 kB 0 B
build/editor/style.css 3.84 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.49 kB 0 B
build/format-library/style-rtl.css 547 B 0 B
build/format-library/style.css 548 B 0 B
build/hooks/index.js 1.74 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.54 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.39 kB 0 B
build/keycodes/index.js 1.85 kB 0 B
build/list-reusable-blocks/index.js 3.02 kB 0 B
build/list-reusable-blocks/style-rtl.css 476 B 0 B
build/list-reusable-blocks/style.css 476 B 0 B
build/media-utils/index.js 5.12 kB 0 B
build/notices/index.js 1.69 kB 0 B
build/nux/index.js 3.27 kB 0 B
build/nux/style-rtl.css 671 B 0 B
build/nux/style.css 668 B 0 B
build/plugins/index.js 2.44 kB 0 B
build/primitives/index.js 1.34 kB 0 B
build/priority-queue/index.js 790 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 13 kB 0 B
build/server-side-render/index.js 2.6 kB 0 B
build/shortcode/index.js 1.69 kB 0 B
build/token-list/index.js 1.24 kB 0 B
build/url/index.js 4.06 kB 0 B
build/viewport/index.js 1.75 kB 0 B
build/warning/index.js 1.13 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@Tug Tug requested a review from lukewalczak October 13, 2020 10:51
Copy link
Member

@lukewalczak lukewalczak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works properly, there is no crash on mobile 🎉

revert_columns

@ntsekouras
Copy link
Contributor

I think the revert could potentially affect other merged PRs (after this one) that had to do with Columns block. For example it will affect this one: #25829.

And this one is I know because I was the author. There could be many more... 🤔

@lukewalczak
Copy link
Member

Hello @ntsekouras, could you please checkout on revert branch and test if your functionality works as expected?

@ntsekouras
Copy link
Contributor

ntsekouras commented Oct 13, 2020

Hello @ntsekouras, could you please checkout on revert branch and test if your functionality works as expected?

It doesn't break it, but it's definitely not correct, as I'm adding a string value and is not set, since it wants number again.

Also

There could be many more... 🤔

Can we find a solution for the mobile problems mentioned here: #24711 (comment), instead?

@lukewalczak
Copy link
Member

lukewalczak commented Oct 13, 2020

Can we find a solution for the mobile problems mentioned here: #24711 (comment), instead?

From my perspective, introducing the UnitControl is really nice feature, but mobile platform is not prepared currently for that, we are not handling units and there is no UI control component for that.

@ockham
Copy link
Contributor

ockham commented Oct 13, 2020

Can we find a solution for the mobile problems mentioned here: #24711 (comment), instead?

From my perspective, introducing the UnitControl is really nice feature, but mobile platform is not prepared currently for that, we are not handling units and there is no UI control component for that.

Any chance we can add those missing features to the mobile platform (possibly at the cost of pushing back a release)? It seems like reverting this PR might otherwise cause us other problems (including block invalidations on websites that have already upgraded to GB 9.1 and where the new version of the Column block has been used).

@lukewalczak
Copy link
Member

I think we can close that PR in favour of temporary workaround.

@Tug Tug closed this Oct 14, 2020
@youknowriad youknowriad deleted the revert-24711 branch October 14, 2020 11:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants