Skip to content

Refactor: BottomSheet controls#1380

Merged
jbinda merged 11 commits intodevelopfrom
callstack/bottomsheet-controls
Oct 2, 2019
Merged

Refactor: BottomSheet controls#1380
jbinda merged 11 commits intodevelopfrom
callstack/bottomsheet-controls

Conversation

@jbinda
Copy link
Copy Markdown
Contributor

@jbinda jbinda commented Sep 25, 2019 •
edited
Loading

Fixes #1365

Please also refer to:
Merge it first gutenberg-mobile PR Feat: Cross platform InspectorControls and Feat: cover cross platform RangeControl
Related gutenberg PR
Related gutenberg-mobile issue

It presents:
New layer on top of BottomSheet.Cells to use controls placed in BottomSheet in the same way as in web version.

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

@jbinda jbinda mentioned this pull request Sep 25, 2019
Merged
5 tasks
@jbinda jbinda requested a review from pinarol September 25, 2019 11:45
@pinarol pinarol requested review from mkevins and removed request for pinarol September 25, 2019 17:55
@pinarol pinarol added the [Type] Enhancement Improves a current area of the editor label Sep 25, 2019
@pinarol pinarol requested a review from etoledom September 27, 2019 11:00
@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Sep 27, 2019

@jbinda is this PR ready for review? It is still marked as draft.

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Sep 27, 2019

@pinarol its in draft because I also need element for Range cell so first I would like have this PR Feat: Cross platform InspectorControls and Feat: cover cross platform RangeControl merged

@etoledom
Copy link
Copy Markdown
Contributor

etoledom commented Oct 1, 2019

This refactor looks quite good!

Since the Range cell is new (afaik), is it really a blocker for this PR? Maybe we could go ahead with this one in parallel.

Unless #1342 is merged soon.

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Oct 1, 2019

I thought it would be nice to merge it all at once but as you said if this refactor is ok we can merge it now and then add Range.Cell implementation

@pinarol
Copy link
Copy Markdown
Contributor

pinarol commented Oct 1, 2019

Agreed, I think we can merge this one first. 👍

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Oct 1, 2019 •
edited
Loading

great! so I will prepare PR to target master in gutenberg repo and remove draft flag

@jbinda jbinda marked this pull request as ready for review October 1, 2019 10:06
etoledom
etoledom approved these changes Oct 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@etoledom etoledom left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good!
Approved via WordPress/gutenberg#17569 (review)

@jbinda
Copy link
Copy Markdown
Contributor Author

jbinda commented Oct 2, 2019 •
edited
Loading

I have revert .gimodules and update ref to master branch and its also ready to merge after CI passed

@ CI passed so I merged it

@jbinda jbinda merged commit 9f01b36 into develop Oct 2, 2019
@etoledom etoledom deleted the callstack/bottomsheet-controls branch October 2, 2019 09:09
@pinarol pinarol added this to the 1.14 milestone Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mkevins mkevins Awaiting requested review from mkevins

1 more reviewer

@etoledom etoledom etoledom approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jbinda jbinda

Labels

[Type] Enhancement Improves a current area of the editor

Projects

None yet

Milestone

1.14

Development

Successfully merging this pull request may close these issues.

Make BottomSheet controls markup consistent with web usage

3 participants

@jbinda @pinarol @etoledom