Skip to content

Refactor: BottomSheet controls #1380

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.

Sign up for GitHub

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

Jump to bottom
Merged
jbinda merged 11 commits into from
Oct 2, 2019
Merged

Refactor: BottomSheet controls #1380

jbinda merged 11 commits into from
Oct 2, 2019

Conversation

@jbinda
Copy link
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
Contributor

pinarol commented Sep 27, 2019

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

@jbinda
Copy link
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
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
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
Contributor

pinarol commented Oct 1, 2019

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

@jbinda
Copy link
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
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
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

4 participants

@jbinda @pinarol @etoledom