#7268: Fix editor sidebar logo sizing #7269
Conversation
| @@ -20,7 +20,4 @@ | |||
| .button { | |||
| background-color: $P500; | |||
| border-color: $P500; | |||
|
|
|||
| display: flex; | |||
| align-items: center; | |||
There was a problem hiding this comment.
Already handled by <Button/>
| onClick={ | ||
| onClick ?? | ||
| (() => { | ||
| dispatch(editorSlice.actions.showHomePane()); |
There was a problem hiding this comment.
The button should unequivocally expand the sidebar, not have a dual behavior.
There was a problem hiding this comment.
Why not just pass the showHomePane action in in SidebarExpanded and make onClick required?
There was a problem hiding this comment.
Because I didn't think of that. 😃
Can do either way. I'll make that change later today
| > | ||
| <img src={home} alt="Return to Page Editor Home" /> | ||
| {children ?? <img src={home} alt="Return to Page Editor Home" />} |
There was a problem hiding this comment.
I thought it'd make sense to just use one icon.
| @@ -44,7 +44,7 @@ const ReloadButton: React.FunctionComponent = () => ( | |||
| className="mt-auto" | |||
| onClick={onReload} | |||
| > | |||
| <FontAwesomeIcon icon={faSync} /> | |||
| <FontAwesomeIcon icon={faSync} fixedWidth /> | |||
There was a problem hiding this comment.
I didn't know it when I added the reload buttons, but this is a better way to make a "square" button, instead of forcing a hard-coded $logoSize
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7269 +/- ##
=======================================
Coverage 71.58% 71.58%
=======================================
Files 1201 1201
Lines 37583 37583
Branches 7055 7055
=======================================
Hits 26904 26904
Misses 10679 10679 ☔ View full report in Codecov by Sentry. |
|
I'm worried about the implementation here. We've removed the opportunity to hit the home button from the collapsed menu. Is there any primary reasoning for swapping out the functionality of the button? @fregante |
Was that intentional? The intent of the two-color button was initially only to expand the sidebar. You can find the original demo in: When the HomeButton was implemented, its nesting did both, IMHO confusingly so: when the sidebar is closed, the most common intent is to expand it, not to lose the selected extension. If that behavior is still preferred, I can restore the two buttons without nesting. |
|
I agree that it was strange that it expanded the menu, but I think the answer is to have it go home without expanding the menu, not eliminating the opportunity for a user to get to the home state while the menu is collapsed. |
|
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
What does this PR do?
In the collapsed sidebar, I dropped the house icon altogether because it doesn't really match the behavior of the button. Also it was the easiest solution instead of keeping the awkward dual-icon dual-color button.
The expanded sidebar is unchanged (except some sizing improvements that reduce the code require for all of this)
Checklist