Fix toolbar issues #3694
Fix toolbar issues #3694
Conversation
5e-Cleric
added
cleanup
There was a problem hiding this comment.
I did this review before cloning the branch to my local machine, so maybe some of the questions are bad, but they are the first things I would check anyway.
Before I left on my trip I was thinking that we were having issues with the toolbar disappearing behind the navbar because of extra re-renders between the toolbar and Brew Renderer components. This PR seems to "take care of it", but doesn't really solve that problem (if it is indeed a problem). And I don't mean to minimize the improvements in this PR-- the absolute positioning is a problem that also needed to be fixed.
I'll check it out tomorrow and see if i can poke it harder
| @@ -3,16 +3,19 @@ | |||
| .brewRenderer { | |||
| overflow-y : scroll; | |||
| will-change : transform; | |||
| padding-top : 30px; | |||
| padding-block : 30px; | |||
There was a problem hiding this comment.
I haven't opened this up locally yet, so maybe this is a stupid question, but if the toolbar is no longer absolutely positioned and overlapping the brewRenderer, we don't need 30px of padding on top at all, right? We only had that extra padding because we wanted to be sure nothing was hidden under the toolbar.
There was a problem hiding this comment.
Because of changes in flow, this padding is all the vertical space between the brew and the top of its parent, whenever you can check out the PR, you will see this space is there for a reason, I changed it for the weird combination of margins in pages and paddings from outside that caused that chaos.
|
Yes, I don't know for sure what causes the extra renderings, but I can take a look, and I should fix this, if I shall find it. |
5e-Cleric
added
the
🔍 R3 - Reviewed - Awaiting Fixes 🔧
|
Cleaned rendering of brewRenderer, cleaned unused CSS from toolbar, blocked on #3682 because page count depends right now on a faulty calculation function. |
5e-Cleric
added
🔍 R4 - Reviewed - Fixed! Awaiting final review⭐
|
I pulled this onto my local branch and the changes seem fine. I don't really think it is 'blocked' by 3682-- the changes here really don't have anything to do with that, unless I'm misunderstanding something. This is mostly just some light CSS and structure changes, and that one is changing how pages are calculated. |
…nto fix-toolbar-issues
…nto fix-toolbar-issues
|
This PR is causing a bug on deployment and local for me, but not for @Gazook89, this small bug requires a bit of testing, to see if it is only on my machine. Check the page numberGrabacion.2024-09-06.223843.mp4Other than that this PR is absolutely ready |
| @@ -139,7 +139,7 @@ const SplitPane = createClass({ | |||
|
|
|||
| render : function(){ | |||
| return <div className='splitPane' onPointerMove={this.handleMove} onPointerUp={this.handleUp}> | |||
| <Pane | |||
| <Pane className="editorPane" | |||
There was a problem hiding this comment.
Should these be named more generic leftPane and rightPane, since they are now being used for /vault, not just editors?
calculuschild
added
🔍 R5 - Approved for merge 💯
🔍 R3 - Reviewed - Awaiting Fixes 🔧
I can't be sure without testing. It makes sense that the error derives from where we calculate the current page, but i need to test that as fix. |
…nto fix-toolbar-issues
|
with #3705 merged, this PR is obsolete, closing |
|
Are these fixes still useful for when the toolbar is currently visisble? |
Not really. |
Changes flow of brewRenderer and splitpane, adds classes to splitpanes, left pane is named
editorPane, and right pane is namedpreviewPane. Grid is used in the pane to establish a defined height for brewRenderer and toolbar.This way, the toolbar can be positioned statically, and the scrollbar does not hide behind it.
Also changed some margins and paddings to make spaces match at the beggining and end of a brew preview.
To anyone reviewing, this PR doesn't make any prominent visual change, other than the scrollbar not going behind the element.