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.

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

Slot/Fill pattern with Toolbar #199

Merged
merged 43 commits into from
Nov 7, 2018
Merged

Slot/Fill pattern with Toolbar #199

merged 43 commits into from
Nov 7, 2018

Conversation

marecar3
Copy link
Contributor

@marecar3 marecar3 commented Oct 26, 2018 •
edited
Loading

Description

This PR resolves part of the Toolbar issue. It adds support for Slot/Fill pattern with Toolbar which results in porting list item Toolbar on the bottom of the screen (or on top of the keyboard).

Slot/Fill pattern with Toolbar

What's done :

  • Fix : is block focused logic on Android platform
  • Implement Slot/Fill "portal" pattern in gutenberg mobile
  • Test Slot/Fill pattern with existing InlineToolbar
  • Create Toolbar container with Format Toolbar Slot and Heading Toolbar Slot on top of the screen
  • Test Slot/Fill pattern with Format Toolbar Slot and existing Format Toolbar

Context (Consumer, Provider) pattern with Toolbar

  • Provide isSelected logic to Toolbar level

screen shot 2018-10-26 at 2 19 54 pm

How has this been tested?

It is tested against this PR

@marecar3 marecar3 added this to the Alpha milestone Oct 26, 2018
@marecar3
Copy link
Contributor Author

marecar3 commented Oct 26, 2018 •
edited
Loading

Update: Currently I am fixing the tests.

koke
koke reviewed Oct 26, 2018
View reviewed changes
Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

Spotted a small typo, and really wanted to try GitHub's new suggest code feature 😁

src/block-management/block-manager.js Outdated
this.props.focusBlockAction( clientId );
}

focusDataSoruceItem(clientId: string) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
focusDataSoruceItem(clientId: string) {
focusDataSourceItem(clientId: string) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

src/block-management/block-manager.js Outdated
@@ -51,9 +52,24 @@ export default class BlockManager extends React.Component<PropsType, StateType>
}

onBlockHolderPressed( clientId: string ) {
this.focusDataSoruceItem(clientId);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.focusDataSoruceItem(clientId);
this.focusDataSourceItem(clientId);

@koke
Copy link
Member

koke commented Oct 26, 2018

Use this branch as gutenberg submodule

If you have your gutenberg directory with that branch (or any other commit checked out), you should see changes in gutenberg when doing git status. You can add and commit those changes so anyone checking out this PR will already have the right Gutenberg version

@marecar3 marecar3 mentioned this pull request Oct 26, 2018
Merged
@hypest
Copy link
Contributor

hypest commented Oct 29, 2018 •
edited
Loading

Nice work @marecar3 !

Tried the PR locally and it seems something goes wrong with the number by the heading buttons (check out the "2", the "3" and the "4" in the Android screenshot below):

screenshot-1540803959602

@marecar3 marecar3 changed the title Feature/toolbar component DO NOT MERGE. Slot/Fill pattern with Toolbar Oct 29, 2018
@marecar3
Copy link
Contributor Author

Hey @koke @hypest , I updated this PR to "DO NOT MERGE" as we shouldn't merge it without implementing propagation of isSelected to Toolbar layer. If we merged it without isSelected nobody will have a chance to use Toolbar without hacking isSelected.

@gziolo
Copy link
Contributor

gziolo commented Oct 30, 2018

You will have to wrap every block with BlockEdit component (https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-edit/index.js) and feed it with the proper value of isSelected based on store.

gziolo
gziolo reviewed Oct 31, 2018
View reviewed changes
src/block-management/block-holder.js
@@ -64,13 +65,15 @@ export default class BlockHolder extends React.Component<PropsType, StateType> {
}

return (
<Block
<BlockEdit
Copy link
Contributor

@gziolo gziolo Oct 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Check also https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/block-list/block.js#L451-L462 for reference. It seems to look good at first glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gziolo !

@marecar3 marecar3 requested a review from etoledom November 2, 2018 09:38
@diegoreymendez diegoreymendez removed their request for review November 2, 2018 11:18
@diegoreymendez
Copy link
Contributor

@marecar3 - I'm removing my assignment for the time being (for transparency) as I won't be able to focus on this PR for at least a few days. Sorry!

@marecar3
Copy link
Contributor Author

marecar3 commented Nov 2, 2018 •
edited
Loading

Hey @diegoreymendez , np, thanks for the update!

koke
koke approved these changes Nov 7, 2018
View reviewed changes
Copy link
Member

@koke koke left a comment

Choose a reason for hiding this comment

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

I approved the Gutenberg PR, so this is good to go after that one is merged and the tests are fixed

@marecar3 marecar3 merged commit 364c786 into master Nov 7, 2018
daniloercoli added a commit that referenced this pull request Nov 7, 2018
@daniloercoli
Merge branch 'master' of https://github.com/wordpress-mobile/gutenber…
4eec6bb 
…g-mobile into fix/block-mergin-remove-red-screen

* 'master' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Slot/Fill pattern with Toolbar (#199)

# Conflicts:
#	gutenberg
daniloercoli added a commit that referenced this pull request Nov 7, 2018
@daniloercoli
Merge branch 'master' of https://github.com/wordpress-mobile/gutenber…
a67c575 
…g-mobile into fix/167-merge-block-loosing-content

* 'master' of https://github.com/wordpress-mobile/gutenberg-mobile:
  Slot/Fill pattern with Toolbar (#199)

# Conflicts:
#	gutenberg
#	yarn.lock
@marecar3 marecar3 deleted the feature/toolbar_component branch January 28, 2019 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

@koke koke koke approved these changes

@hypest hypest Awaiting requested review from hypest

@etoledom etoledom Awaiting requested review from etoledom

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Alpha
Development

Successfully merging this pull request may close these issues.
6 participants
@marecar3 @koke @hypest @gziolo @diegoreymendez @daniloercoli