-
Notifications
You must be signed in to change notification settings - Fork 129
Drop hover width based off of 100%, subtract depth #3373
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
base: main
Are you sure you want to change the base?
Drop hover width based off of 100%, subtract depth #3373
Conversation
|
Sorry I should have said content width, not screen width. As far as I know the snapshots are correct, but let me know if there are any that seem wrong to you and I'd be happy to discuss. Another approach would be to make the drop hover fixed width. This would result in the correct stepped edge on the right as the depth increased. But in order to support the deepest thoughts, the fixed width would have to be quite small. This is why I was thinking it ought to scale by the max visible depth. |
|
I'm made some progress on this, but there are still 8-10 failing tests. I reviewed them to see if there's a common thread, but it's not looking great. There are a variety of issues including simple things like a couple drop hovers that are still too long or short, but there are also drop hovers that are showing up with the wrong indentation, and drop targets that seem to be missing entirely or else are covered by another drop target and are not selectable. Given the limited scope of these changes, I'm not even sure how a drop target would go missing, nor am I quite sure how much longer it would take to track down the sources of all of these issues, but my guess is quite a while. |
I think it would be a good idea to assess these complications and determine if the overall approach is still viable and does not introduce additional complexity. If there are too many edge cases to address, we may inadvertently make the system fragile in different ways. In theory, it doesn't seem like a depth subtraction-based width is that complex. So, there must be some hidden dependencies that we haven't yet clearly articulated. What else does the drop hover width depend on, or vice versa? The drop-hover element itself is position absolute, so it shouldn't be affecting the layout. Is it tied to the drop target in a different way that prevents it from being adjusted independently? Perhaps take a step back and consider some of these more general questions about the drop hover and how it interacts with the layout engine and drag-and-drop functionality. Hopefully that helps give you a clearer vision of what limitations we are working with and what the best way is to move forward. Thanks! |
Fixes #3369
I updated useDropHoverWidth to start from the basic calculation that should match
contentWidth,calc(100% - 2em - ${fontSize}px + 10px), which is the inverse of how TreeNode width is calculated with themarginLeftfrom dropEndRecipe added in. Then I started trying to offset the hover targets for thoughts that are not the max visible depth. I needed to calculate maxDepth. You pointed me in the right direction with100% - padding - depth * indentSize, but it's actually the inverse of depth because the deepest depth (maxDepth) gets 0px subtracted from it, and then every shallower child gets1em * (maxDepth - depth)subtracted.That went pretty well, but then I started to run into edge cases with DropEnd and DropHover and all the rest. It will still take some digging to match all of the snapshots, although hopefully I'll be able to simplify my implementation in the process. Let me know if I should be trying to accomplish something other than matching snapshots, as I'm not sure they always follow the rule that you outlined, "The goal is for the drop hover with the deepest path to extend to the right edge of the screen, and then for shallower paths to be shortened by the difference in indentation." I figure that once the browser passes above 1200px, the thoughts don't ever actually stretch all the way to the right, and that may be OK. Historically, all drop targets are
contentWidthpixels wide, and haven't been adjusted to match their various margins.