-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Transform Blocks #28453
Transform Blocks #28453
Conversation
packages/block-editor/src/components/block-mobile-toolbar/block-actions-menu.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-mobile-toolbar/block-actions-menu.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-mobile-toolbar/block-actions-menu.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-mobile-toolbar/block-actions-menu.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-switcher/block-transformations-menu.native.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be working great already! Nice work @illusaen. I had a follow-up question above but I think this is on a good path.
Moving white/blacklist to transforms.native.js is taking longer than expected; will come back to this after Simplenote/groundskeeping rotations. |
…red options picker for block settings to have options in one object for better filtering and separated out props to the three different types.
…ell if there are no valid transformation options and if so, not pop up transform option.
…ock depending on if they are allowed to transform.
…block transformation categories in.
…forming group -> column block that resulted in crash.
@@ -60,7 +60,7 @@ export class BlockList extends Component { | |||
renderFooterAppender: this.props.renderFooterAppender, | |||
renderAppender: this.props.renderAppender, | |||
onDeleteBlock: this.props.onDeleteBlock, | |||
contentStyle: this.props.contentstyle, | |||
contentStyle: this.props.contentStyle, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is an unrelated (bug?) I found that doesn't seem to change anything when changed. However, contentstyle
(original with no caps) ONLY appears in this one case, while contentStyle
appears everywhere else (and is the actual prop for the RN View
).
@@ -52,6 +52,10 @@ function ColumnEdit( { | |||
clientId, | |||
blockWidth, | |||
} ) { | |||
if ( ! contentStyle ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: When transforming a group -> column, contentStyle
is undefined, which leads to a crash. I added this little kludge and the transformation magically works as the web does, meaning that the width in contentStyle
doesn't actually matter/is reset?
I'm 100% open to ideas as to WHY contentStyle
is undefined when passing through createBlock
even though my debugging shows that creating a new column (which somehow gives a contentStyle
to column/edit.native.js
) doesn't set contentStyle
anywhere in its props
/attributes
/etc either.
However, I'm not sure that this is a bug that should be something fixed in this PR.
@illusaen This is looking pretty solid and should set us up nicely for the next iteration of the UI, nice work! I tested out the latest test build on iOS and broke my feedback down into 2 sections (plus one indirectly-related “bonus” bug that I discovered while testing): things that are mobile-specific and things that aren’t really “bugs” because behavior matches that of the web UI. Mobile-specific things
Not mobile specificThese are things that also apply to the web editor, so I wouldn’t necessarily consider them ”bugs”:
Unrelated
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work on this @illusaen 👍
I like how you handled the supportedMobileTransforms
and grouped them in the transformationCategories
.
The code looks good and I didn't have anything to add in addition to @iamthomasbishop's design review.
@iamthomasbishop Thanks! :)
Done, see new GIF attached below :)
Unfortunately not possible to add images to
So for the items above, I'm using directly web's transformation functions. I could add the functionality but it would require kludging it, so I'm not sure if we should discuss just changing the behaviour for both platforms on a separate thread? Edit: I 100% agree with these ideas on UX btw. I also found it weird that transforming Group -> Column results in that group wrapped in a column instead of turning each item in the group into a column.
Looking into this now!
Framework for this added; just need list of items to add to it :) In new GIF (attached below) should be able to see the one I added for testing purposes, List > Paragraph.
Added as bug.
Added as bug. |
@iamthomasbishop I think I've gotten the changes in; planning to merge at EOD today. If you see something please feel free to tell me and I'll make a separate PR for it! |
Description
Fixes #2814
Allow users to transform blocks that are:
NOTE: Mobile supports a subset of the web transformations; i.e. we don't support any that the web doesn't, and we don't support all of the web's transformations. Thus, when following the diagram below, the transformations don't match exactly as the web doesn't support all of the transformations on the diagram.
Transformation Category Diagram
How has this been tested?
Paragraph (allowed)
RSS (not allowed)
Screenshots
New:
Screen.Recording.2021-03-30.at.3.49.43.PM.mov
Old GIFs
Types of changes
Checklist: