[RNMobile] Add Group and Ungroup block actions #50693
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Most of the logic of this hook has been extracted from `ConvertToGroupButton` component. The main difference is that we return the configuration for the block actions instead of a component.
fluiddot
added
the
Mobile App - i.e. Android or iOS
|
Size Change: 0 B Total Size: 1.39 MB ℹ️ View Unchanged
|
fluiddot
requested review from
SiobhyB,
geriux and
ntsekouras
and removed request for
ajitbohra
|
With these two new options, the block actions menu is getting a bit long. Maybe we could consider in the future reduce it by adding submenus. cc @osullivanchris |
4 tasks
|
@fluiddot the action sheet on iOS is starting to feel dated to me, as a component we use across the app. The iOS context menu is more condense and has different display options, so could manage this better. Even on Android we're using our own bottom sheet here, which is more condense than the action sheet. So could be an alternative option. I think it would be hard to provide two levels, particularly in the action sheet. As it would be hard to come up with a sensible name for any group of the actions. E.g. you could put copy, cut and duplicate into a group - but it would be very hard to come up with a clear name for that group. |
|
Thanks @osullivanchris for the feedback 🙇 !
Yep, I agree. It would be great to revamp it eventually. I hope we can focus on this in a next project 🤞 .
I found this library that might help us to start incorporating contextual menus in React Native: It could be a good starter to explore how to replace the action sheets.
True. In the web version, the block actions menu is also long. But obviously, we have plenty more space there. However, observe that some items are grouped in sections. 
|
|
@fluiddot yeah the context menu would be nice to explore at some point. Ah I misunderstood about groups. I thought you meant to have another layer of hierarchy as in group them into a single item, and take an extra step/tap. I see what you mean now. I have not used groups in action sheets before, but if its a pattern that we support I'm fine having subgroups here. If we don't support it already, I'm not sure its worthwhile adding a new variation of an action sheet. I'd rather we spend the effort to move to a different UI altogether like context menu or bottom sheet. |
Yeah, actually I meant that 😅. My comment was related to seeing what options we have to reduce the size of the block actions menu, but I didn't want to reference anything in particular.
I think the only subgroup pattern we currently have in action sheets is opening another sheet. As an example, you can check this when tapping on the "Transform ..." option. However, this option doesn't allow navigating back. As you mentioned, I agree that at this point, it would be great to explore a more solid approach like context menus. |
|
@fluiddot ah ok. So I was right the first time. As I said I just can't think of a sensible name for those groups of actions. I think we can just park it for now, accept the list is getting a bit too big, and its another data point to say we need a more modern component 😄 |
|
👋 Thanks for the PR! Is something blocking this PR? I get that you're discussing about exploring submenus in follow ups, no? It could be great to land this one and unblock this one. |
I think the design discussion is not really blocking the PR, any potential work in this regard will be done in the future. The main blocker at this point is the review and approval of the PR. I'll ping reviewers in case the PR went unnoticed. |
SiobhyB
approved these changes
May 19, 2023
There was a problem hiding this comment.
@fluiddot, LGTM! I agree that it'd be good to revisit the UI in the future, perhaps as part of our upcoming UX improvement project :)
Functionally, the grouping/ungrouping works as expected on both Android and iOS, matching up to the web's behaviour. I don't see any blockers to this PR. Thanks for working on this, Carlos! 👏 🙌
ntsekouras
added a commit
that referenced
this pull request
May 19, 2023
) * Remove `unwrap` from transforms and add `ungroup` to more blocks * add docs * update e2e tests * add e2e test * Refactor useConvertToGroupButtonProps for readability * Apply suggestions from code review Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com> * update docs * reword ungrouping in docs * update e2e test * [RNMobile] Add Group and Ungroup block actions (#50693) * Add `useConvertToGroupButtons` hook Most of the logic of this hook has been extracted from `ConvertToGroupButton` component. The main difference is that we return the configuration for the block actions instead of a component. * Add Group and Ungroup options to block actions menu * Remove `canUnwrap` option from `getBlockTransformOptions` test helper * Update tests for Group, Quote and Columns blocks --------- Co-authored-by: Miguel Fonseca <miguelcsf@gmail.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR incorporates the Group and Ungroup block actions to match the web version.
Why?
Currently, grouped blocks can be unwrapped via the
unwraptransform option. This option will be removed in #50385 in favor of Ungroup block action. Hence, we need to incorporate it in the native version.How?
useConvertToGroupButtonsthat returns the Group and Ungroup options to use in the block actions menu. The logic for this hook is very similar to theConvertToGroupButtoncomponent, the main difference is that the latter returns a component.Testing Instructions
Group/Ungroup
Ungroup is not displayed on empty groups
Unwrap transform is not displayed
Group,Quote,Columnsblocks.unwrapoption is not displayed.Testing Instructions for Keyboard
N/A
Screenshots or screencast