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.

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

Add/346 list block/ui #382

Closed
wants to merge 70 commits into from
Closed

Add/346 list block/ui #382

wants to merge 70 commits into from

Conversation

intronic
Copy link
Contributor

@intronic intronic commented Apr 6, 2017

Key differences:

  • after our experience on the prototype, we decided some minimum relevant state so far to render the UI is the currently active block and its bounding rect. We didnt yet find a need for state information about unfocused blocks while drawing the UI.

  • because of this we opted to add a new slot in the state under 'blocks' called 'focused' where we record the UID and bounding rect of the currently focused block (or null, if the overall Layout no longer has focus).

  • the new actions are FOCUS_BLOCK (when a block receives focus) and FOCUS_LOST when everything in the layout has lost focus

  • we didnt want to change any existing state or reducers, to see how it will all shake out

  • We havent compared this yet to the latest PR from @aduth , which we will do shortly. We seem to have a race condition here :) - similar ideas, slightly different execution. It would be good to work out how to coordinate so we reduce duplicated effort. A bit tricky with so many timezones.

  • we added an alignment toolbar under controls, and a wrapper for tinymce blocks that wires up the buttons to the tiny api

  • we added a component AbsolutePosition that wraps things in a positioned div

  • we refactored a few connect('inlne stuff')() calls to be connect( mapStateToProps, mapDispatchToProps )( ); just because it seemed easier to remember the meaning of the params

  • there were several merge conflicts with master this morning, I hope we resolved it all OK, seems like a few state things were removed/changed

  • The PR has a lot of files, but many are to do with dashicons we pulled in from the tinymce-per-block prototype.

Cheers from @mimo84 & I

mimo84 and others added 3 commits April 7, 2017 09:11
# Conflicts:
#	blocks/components/editable/index.js
#	editor/assets/stylesheets/main.scss
#	editor/state.js
#	element/index.js
@intronic
Copy link
Contributor Author

intronic commented Apr 6, 2017

oh - we'll fix up the CI stuff, didnt notice that

@intronic
Copy link
Contributor Author

intronic commented Apr 6, 2017

also, should we follow the react-redux patterns to split out the connect()() code into container Components separately from the stateless UI component code?

@aduth
Copy link
Member

aduth commented Apr 7, 2017

It's a little hard to review the changes because I think it could do for a rebase and force push. Ping me on Slack if you need a hand with this.

similar ideas, slightly different execution. It would be good to work out how to coordinate so we reduce duplicated effort. A bit tricky with so many timezones.

Agreed. If you find I'm pulling too far away from where you're trying to execute, please drop a message in Slack #core-editor. I'd love to talk through these ideas.

In general though, the smaller we keep pull requests, the less chance we'll run into conflicts and drift.

add PropTypes for validation

I avoided (and documented avoidance) PropTypes because Facebook themselves now use and encourage Flow instead, which makes me very skeptical of its future.

we refactored a few connect('inlne stuff')() calls to be connect( mapStateToProps, mapDispatchToProps )( ); just because it seemed easier to remember the meaning of the params

Sure, I think this is a common enough sentiment that it makes sense to build a convention around it.

also, should we follow the react-redux patterns to split out the connect()() code into container Components separately from the stateless UI component code?

I don't think we should. They are already separate components (connect returns a new component). It's just that they live in the same file. I've gone down this path in the past and it turns out that if we want almost every component to be individually connected to retrieve the specific state they're concerned with, the additional boilerplate of separate files becomes cumbersome and confusing (especially around naming). I'd rather encourage granular use of connect than multi-purpose all-encompassing container components.

@intronic intronic added the Framework Issues related to broader framework topics, especially as it relates to javascript label Apr 8, 2017
@intronic
Copy link
Contributor Author

intronic commented Apr 8, 2017

OK I think @mimo84 & I will refactor this to take into account the comments above, recent merges, rebase it and force-push as suggested. Will ping you later on core-editor @aduth .

@jasmussen
Copy link
Contributor

I see a list block in master!

Does that mean this PR can be closed, or is it still alive?

@ellatrix ellatrix added this to the Prototype Parity milestone Apr 26, 2017
@BoardJames
Copy link

@jasmussen This can be closed. It was superseded and replaced when the UI framework was added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants