-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Reveal block controls on hover #59
Conversation
Nice work! You can't use the switcher or the up/down arrows though: Also, we shouldn't show the block level controls until you actually click the block. Here's a mockup of the hover style without the block controls: https://cloud.githubusercontent.com/assets/1204802/22922702/20f18958-f29f-11e6-9aab-9d1cb7f042b3.png The key issue here is: you've written your post, inserted your content, and you're entering the layout bout of your blogging session. You might be moving around the images a bit, or converting select paragraphs to blockquotes. Doing that through hovering over the blocks with a few direct clicks on the side controls, possibly, allows for a faster more convenient feel. |
Yeah, it's pretty broken at the moment :). I have a few more changes locally which I'll try to share soon, along with some other observations.
💯 |
Small status update that was missing from my previous comment: The issue of the switcher disappearing when moving the mouse towards it could instinctively be addressed by expanding the block's ( My point behind this verbose recap is that I think that we're pushing the limits of what we can achieve with the prototype's current DOM structure — namely, the fact that the DOM nodes of both the switcher and the controls are not descendants of a block's container. I can try and work this one out solely with some added stateful logic, but a restructuring may be due. /cc @jasmussen @mtias |
I think that may be time regardless, if we are to fix #53 in a nice way. Incidentally it begs another question, which ties into #51 — how far do we take this? Should we keep it up to snuff, perhaps even morph it into something more than just a UI prototype? Or should we consider it good enough for getting a feel for the block controls and UI, and move on to one of the bigger prototypes suggested here? |
Hadn't spotted #51 yet — glad we're having that discussion!
So far work on this prototype has always been implemented in what I think has been a refreshing short cycle of "expand functionality, refactor minimally". Given my tendencies to over-architect, it's great to keep that up and it has helped us getting the ideas out there by focusing on sharing prototype improvements quickly — rather than being blocked by concerns of code correctness or trying to tackle all the little broken things. In that light, I would take this only as far as rigorously necessary. For instance, that could mean that, instead of having a sophisticated way to dynamically move the switcher+controls into each block as it gets selected, we would just have an initial pass that clones the switcher+controls and appends a clone to each block — we would get per-block "state" for free in the form of DOM nodes. |
I'm seeing some odd behaviours after moving a block (the controls disappear), and sometimes it just stops working after moving things around a bit. |
Yeah, it's still buggy. I had mentioned that in the latest commit's message but forgot to mention it here. |
b6cde3c
to
e5f8602
Compare
Force pushed a rebase to resolve conflicts. Last commit e5f8602 includes an updated approach, creating a duplicate standalone switcher for use on hover instead of cloned switcher controls for each block (previously fd2f9b4). This way there can be switcher controls for both the selected block and, separately, a hovered block, while also not relying on any particular DOM structure of the editor. They selected and hover switchers behave similarly, so refactoring was needed to remove assumptions about there being a single set of switcher controls. Also accounts for updated mockup to apply border on one side of block for hover. Not the prettiest code admittedly, but par for the course with the current setup I'd say 😄 Might also still need a little polish with edge cases around refreshing whilst hovered, combinations of hover/selecting interactions with switcher actions. |
If I select a block, I can't move it. Moving a block (on hover) doesn't update the scroll position. Also, it seems moving a block should also leave it selected after moving? |
- Allows on-hover reveal of switcher to happen reliably with CSS - Requires refactoring of a few event-binding functions in order for them to operate on blocks rather than on a singleton node. - Reliance on the `selectedBlock` global is diminished. The drawback is a more hybrid code base, where some logic expects `selectedBlock` and some doesn't need it. The advantage is that less mutating state needs to be managed (setting `selectedBlock` can be delayed up until the point it is necessary for a given operation). - A couple of TypeErrors may pop up in the console. I suspect there are some interactions where the target block isn't defined. Needs to be addressed.
e5f8602
to
1a6a8fb
Compare
Reimplements swapNodes to rearrange children in place instead of creating clones
1a6a8fb
to
249f6f1
Compare
Should be fixed in b1dc354.
Should be fixed in 249f6f1.
This should have been working, or are you thinking moving a hovered block should have it become selected? Which raises another question; should moving a hovered block (particularly while another is selected) result in the viewport offset changing? |
Good questions. I found that I lost my placement a bit if the moved block is not selected when clicking on the arrow. For dragging it seems it wouldn't be much of an issue since I'll be positioning my attention where I want to drop the block. |
Closing this for now. |
Update example project to use RN 0.57
Fixes #58